[PATCH] D143847: [RISCV] Add vendor-defined XTheadMAC (multiply-accumulate) extension

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVFeatures.td:491
+def HasVendorXTHeadMac : Predicate<"Subtarget->hasVendorXTHeadMac()">,
+                                  AssemblerPredicate<(all_of FeatureVendorXTHeadMac),
+                                  "'xtheadmac' (T-Head Multiply-Accumulate Instructions)">;
----------------
Identation is off here


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoXTHead.td:96
+		    opcodestr, "$rd, $rs1, $rs2"> {
+  let Constraints = "$rd_up = $rd";
+}
----------------
I think we usually use `$rd_wb` for the destination on tied instructions


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoXTHead.td:360
+// mulaw, mulsw are available only in RV64.
+def : Pat<(sext_inreg (add GPR:$rd, (mul GPR:$rs1, GPR:$rs2)), i32), (TH_MULAW GPR:$rd, GPR:$rs1, GPR:$rs2)>;
+def : Pat<(sext_inreg (sub GPR:$rd, (mul GPR:$rs1, GPR:$rs2)), i32), (TH_MULSW GPR:$rd, GPR:$rs1, GPR:$rs2)>;
----------------
You can probably use `binop_allwusers<add>` instead of (sext_inreg (add ...)). Similar for sub. Then add these instructions to the switch in RISCVDAGToDAGISel::doPeepholeSExtW with ADDIW/ADDW/etc.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoXTHead.td:363
+// mulah, mulsh need an outer (sext_inreg ..., i32) in RV64.
+def : Pat<(sext_inreg (add GPR:$rd, (mul (sext_inreg GPR:$rs1, i16), (sext_inreg GPR:$rs2, i16))), i32), (TH_MULAH GPR:$rd, GPR:$rs1, GPR:$rs2)>;
+def : Pat<(sext_inreg (sub GPR:$rd, (mul (sext_inreg GPR:$rs1, i16), (sext_inreg GPR:$rs2, i16))), i32), (TH_MULSH GPR:$rd, GPR:$rs1, GPR:$rs2)>;
----------------
Also better to use binop_allwusers here.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoXTHead.td:365
+def : Pat<(sext_inreg (sub GPR:$rd, (mul (sext_inreg GPR:$rs1, i16), (sext_inreg GPR:$rs2, i16))), i32), (TH_MULSH GPR:$rd, GPR:$rs1, GPR:$rs2)>;
+// Also try match for mulah, mulsh in the absense of Zbb.
+def : Pat<(sext_inreg (add GPR:$rd, (mul
----------------
"Zbb" -> "Zbb or THeadBb"?


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoXTHead.td:379
+def : Pat<(sub GPR:$rd, (mul (sext_inreg GPR:$rs1, i16), (sext_inreg GPR:$rs2, i16))), (TH_MULSH GPR:$rd, GPR:$rs1, GPR:$rs2)>;
+// Also try match for mulah, mulsh in the absense of Zbb.
+def : Pat<(add GPR:$rd, (mul
----------------
Same here


================
Comment at: llvm/test/CodeGen/RISCV/attributes.ll:95
 ; RUN: llc -mtriple=riscv64 -mattr=+xtheadbs %s -o - | FileCheck --check-prefixes=CHECK,RV64XTHEADBS %s
+; RUN: llc -mtriple=riscv64 -mattr=+xtheadMAC %s -o - | FileCheck --check-prefixes=CHECK,RV64XTHEADMAC %s
 ; RUN: llc -mtriple=riscv64 -mattr=+xtheadvdot %s -o - | FileCheck --check-prefixes=CHECK,RV64XTHEADVDOT %s
----------------
xtheadMAC -> xtheadmac


================
Comment at: llvm/test/MC/RISCV/rv32xtheadmac-invalid.s:3
+
+th.mulaw  t0, t1, t2     # CHECK: :[[@LINE]]:1: error: instruction requires the following: RV64I Base
+th.mulsw  t0, t1, t2     # CHECK: :[[@LINE]]:1: error: instruction requires the following: RV64I Base
----------------
Add `{{$}}` to the end of the CHECK to make sure there is nothing else listed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143847/new/

https://reviews.llvm.org/D143847