[RISCV] Sink hasSideEffects, mayLoad, mayStore from defs to classes in RISCVInstrInfoXCV.td. NFC #130714

topperc · 2025-03-11T03:56:26Z

This is consistent with how RISCVInstrInfo.td is generally structured.

…n RISCVInstrInfo.td. NFC This is consistent with how RISCVInstrInfo.td is generally structured.

llvmbot · 2025-03-11T03:56:59Z

@llvm/pr-subscribers-backend-risc-v

Author: Craig Topper (topperc)

Changes

This is consistent with how RISCVInstrInfo.td is generally structured.

Full diff: https://github.com/llvm/llvm-project/pull/130714.diff

1 Files Affected:

(modified) llvm/lib/Target/RISCV/RISCVInstrInfoXCV.td (+31-30)

diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfoXCV.td b/llvm/lib/Target/RISCV/RISCVInstrInfoXCV.td
index 4cf0bb94af347..11670380e705a 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfoXCV.td
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfoXCV.td
@@ -10,7 +10,8 @@
 //
 //===----------------------------------------------------------------------===//
 
-let DecoderNamespace = "XCV" in {
+let DecoderNamespace = "XCV",
+    hasSideEffects = 0, mayLoad = 0, mayStore = 0 in {
   class CVInstBitManipRII<bits<2> funct2, bits<3> funct3, dag outs, dag ins,
                       string opcodestr, string argstr>
       : RVInstIBase<funct3, OPC_CUSTOM_2, outs, ins, opcodestr, argstr> {
@@ -36,10 +37,9 @@ let DecoderNamespace = "XCV" in {
                 (ins GPR:$rs1), opcodestr, "$rd, $rs1"> {
     let rs2 = 0b00000;
   }
-}
+} // DecoderNamespace = "XCV", hasSideEffects = 0, mayLoad = 0, mayStore = 0
 
-let Predicates = [HasVendorXCVbitmanip, IsRV32],
-    hasSideEffects = 0, mayLoad = 0, mayStore = 0 in {
+let Predicates = [HasVendorXCVbitmanip, IsRV32] in {
   def CV_EXTRACT : CVBitManipRII<0b00, 0b000, "cv.extract">;
   def CV_EXTRACTU : CVBitManipRII<0b01, 0b000, "cv.extractu">;
 
@@ -54,7 +54,8 @@ let Predicates = [HasVendorXCVbitmanip, IsRV32],
     def CV_INSERT : CVInstBitManipRII<0b10, 0b000, (outs GPR:$rd_wb),
                              (ins GPR:$rd, GPR:$rs1, uimm5:$is3, uimm5:$is2),
                              "cv.insert", "$rd, $rs1, $is3, $is2">;
-    let DecoderNamespace = "XCV" in
+    let DecoderNamespace = "XCV",
+        hasSideEffects = 0, mayLoad = 0, mayStore = 0 in
     def CV_INSERTR : RVInstR<0b0011010, 0b011, OPC_CUSTOM_1, (outs GPR:$rd_wb),
                              (ins GPR:$rd, GPR:$rs1, GPR:$rs2),
                              "cv.insertr", "$rd, $rs1, $rs2">;
@@ -70,6 +71,7 @@ let Predicates = [HasVendorXCVbitmanip, IsRV32],
   def CV_CNT : CVBitManipR<0b0100100, "cv.cnt">;
 }
 
+let hasSideEffects = 0, mayLoad = 0, mayStore = 0 in
 class CVInstMac<bits<7> funct7, bits<3> funct3, string opcodestr>
     : RVInstR<funct7, funct3, OPC_CUSTOM_1,
               (outs GPR:$rd_wb), (ins GPR:$rd, GPR:$rs1, GPR:$rs2),
@@ -89,6 +91,7 @@ class CVInstMacMulN<bits<2> funct2, bits<3> funct3, dag outs, dag ins,
   let DecoderNamespace = "XCV";
 }
 
+let hasSideEffects = 0, mayLoad = 0, mayStore = 0 in {
 class CVInstMacN<bits<2> funct2, bits<3> funct3, string opcodestr>
     : CVInstMacMulN<funct2, funct3, (outs GPR:$rd_wb),
                     (ins GPR:$rd, GPR:$rs1, GPR:$rs2, uimm5:$imm5), opcodestr> {
@@ -98,9 +101,9 @@ class CVInstMacN<bits<2> funct2, bits<3> funct3, string opcodestr>
 class CVInstMulN<bits<2> funct2, bits<3> funct3, string opcodestr>
     : CVInstMacMulN<funct2, funct3, (outs GPR:$rd),
                     (ins GPR:$rs1, GPR:$rs2, uimm5:$imm5), opcodestr>;
+} // hasSideEffects = 0, mayLoad = 0, mayStore = 0
 
-let Predicates = [HasVendorXCVmac, IsRV32], hasSideEffects = 0, mayLoad = 0,
-    mayStore = 0 in {
+let Predicates = [HasVendorXCVmac, IsRV32] in {
   // 32x32 bit macs
   def CV_MAC      : CVInstMac<0b1001000, 0b011, "cv.mac">,
                     Sched<[]>;
@@ -126,9 +129,7 @@ let Predicates = [HasVendorXCVmac, IsRV32], hasSideEffects = 0, mayLoad = 0,
                     Sched<[]>;
   def CV_MACHHURN : CVInstMacN<0b11, 0b111, "cv.machhurn">,
                     Sched<[]>;
-} // Predicates = [HasVendorXCVmac, IsRV32], hasSideEffects = 0, mayLoad = 0...
 
-let Predicates = [HasVendorXCVmac, IsRV32], hasSideEffects = 0, mayLoad = 0, mayStore = 0 in {
   // Signed 16x16 bit muls with imm
   def CV_MULSN    : CVInstMulN<0b00, 0b100, "cv.mulsn">,
                     Sched<[]>;
@@ -148,9 +149,7 @@ let Predicates = [HasVendorXCVmac, IsRV32], hasSideEffects = 0, mayLoad = 0, may
                     Sched<[]>;
   def CV_MULHHURN : CVInstMulN<0b11, 0b101, "cv.mulhhurn">,
                     Sched<[]>;
-} // Predicates = [HasVendorXCVmac, IsRV32], hasSideEffects = 0, mayLoad = 0...
 
-let Predicates = [HasVendorXCVmac, IsRV32] in {
   // Xcvmac Pseudo Instructions
   // Signed 16x16 bit muls
   def : InstAlias<"cv.muls $rd1, $rs1, $rs2",
@@ -165,7 +164,8 @@ let Predicates = [HasVendorXCVmac, IsRV32] in {
                   (CV_MULHHUN GPR:$rd1, GPR:$rs1, GPR:$rs2, 0)>;
 } // Predicates = [HasVendorXCVmac, IsRV32]
 
-let DecoderNamespace = "XCV" in {
+let DecoderNamespace = "XCV",
+    hasSideEffects = 0, mayLoad = 0, mayStore = 0 in {
   class CVInstAluRRI<bits<2> funct2, bits<3> funct3, string opcodestr>
       : RVInstRBase<funct3, OPC_CUSTOM_2, (outs GPR:$rd),
                     (ins GPR:$rs1, GPR:$rs2, uimm5:$imm5), opcodestr,
@@ -204,8 +204,7 @@ let DecoderNamespace = "XCV" in {
 
 } // DecoderNamespace = "XCV"
 
-let Predicates = [HasVendorXCValu, IsRV32],
-  hasSideEffects = 0, mayLoad = 0, mayStore = 0 in {
+let Predicates = [HasVendorXCValu, IsRV32] in {
   // General ALU Operations
   def CV_ABS    : CVInstAluR<0b0101000, 0b011, "cv.abs">,
                   Sched<[]>;
@@ -255,11 +254,7 @@ let Predicates = [HasVendorXCValu, IsRV32],
                   Sched<[]>;
   def CV_SUBURN : CVInstAluRRI<0b11, 0b011, "cv.suburn">,
                   Sched<[]>;
-} // Predicates = [HasVendorXCValu, IsRV32],
-  //   hasSideEffects = 0, mayLoad = 0, mayStore = 0
 
-let Predicates = [HasVendorXCValu, IsRV32],
-  hasSideEffects = 0, mayLoad = 0, mayStore = 0 in {
   def CV_ADDNR   : CVInstAluRRNR<0b1000000, 0b011, "cv.addnr">,
                    Sched<[]>;
   def CV_ADDUNR  : CVInstAluRRNR<0b1000001, 0b011, "cv.addunr">,
@@ -277,8 +272,7 @@ let Predicates = [HasVendorXCValu, IsRV32],
   def CV_SUBURNR : CVInstAluRRNR<0b1000111, 0b011, "cv.suburnr">,
                    Sched<[]>;
 
-} // Predicates = [HasVendorXCValu, IsRV32],
-  //   hasSideEffects = 0, mayLoad = 0, mayStore = 0,
+} // Predicates = [HasVendorXCValu, IsRV32]
 
 let Predicates = [HasVendorXCValu, IsRV32] in {
   def : MnemonicAlias<"cv.slet", "cv.sle">;
@@ -307,6 +301,7 @@ class CVInstSIMDRI<bits<5> funct5, bit F, bits<3> funct3, RISCVOpcode opcode,
   let DecoderNamespace = "XCV";
 }
 
+let hasSideEffects = 0, mayLoad = 0, mayStore = 0 in {
 class CVSIMDRR<bits<5> funct5, bit F, bit funct1, bits<3> funct3,
                string opcodestr>
     : CVInstSIMDRR<funct5, F, funct1, funct3, OPC_CUSTOM_3, (outs GPR:$rd),
@@ -350,6 +345,7 @@ class CVSIMDR<bits<5> funct5, bit F, bit funct1, bits<3> funct3,
                   (ins GPR:$rs1), opcodestr, "$rd, $rs1"> {
   let rs2 = 0b00000;
 }
+} // hasSideEffects = 0, mayLoad = 0, mayStore = 0
 
 multiclass CVSIMDBinarySigned<bits<5> funct5, bit F, bit funct1, string mnemonic> {
   def CV_ # NAME # _H : CVSIMDRR<funct5, F, funct1, 0b000, "cv." # mnemonic # ".h">;
@@ -397,8 +393,7 @@ multiclass CVSIMDBinaryUnsignedWb<bits<5> funct5, bit F, bit funct1, string mnem
 }
 
 
-let Predicates = [HasVendorXCVsimd, IsRV32],
-    hasSideEffects = 0, mayLoad = 0, mayStore = 0 in {
+let Predicates = [HasVendorXCVsimd, IsRV32] in {
   defm ADD :    CVSIMDBinarySigned<0b00000, 0, 0, "add">;
   defm SUB :    CVSIMDBinarySigned<0b00001, 0, 0, "sub">;
   defm AVG :    CVSIMDBinarySigned<0b00010, 0, 0, "avg">;
@@ -495,16 +490,18 @@ let Predicates = [HasVendorXCVsimd, IsRV32],
   def CV_SUB_DIV8 :    CVSIMDRR<0b01110, 1, 0, 0b110, "cv.sub.div8">;
 }
 
+let hasSideEffects = 0, mayLoad = 0, mayStore = 0 in
 class CVInstImmBranch<bits<3> funct3, dag outs, dag ins,
                       string opcodestr, string argstr>
     : RVInstB<funct3, OPC_CUSTOM_0, outs, ins, opcodestr, argstr> {
   bits<5> imm5;
   let rs2 = imm5;
+  let isBranch = 1;
+  let isTerminator = 1;
   let DecoderNamespace = "XCV";
 }
 
-let Predicates = [HasVendorXCVbi, IsRV32], hasSideEffects = 0, mayLoad = 0,
-    mayStore = 0, isBranch = 1, isTerminator = 1 in {
+let Predicates = [HasVendorXCVbi, IsRV32] in {
   // Immediate branching operations
   def CV_BEQIMM : CVInstImmBranch<0b110, (outs),
         (ins GPR:$rs1, simm5:$imm5, simm13_lsb0:$imm12),
@@ -530,15 +527,18 @@ def CVrr : Operand<i32>,
    let MIOperandInfo = (ops GPR:$base, GPR:$offset);
 }
 
+let hasSideEffects = 0, mayLoad = 1, mayStore = 0 in {
 class CVLoad_ri_inc<bits<3> funct3, string opcodestr>
-    : RVInstI<funct3, OPC_CUSTOM_0, (outs GPR:$rd, GPR:$rs1_wb), (ins GPRMem:$rs1, simm12:$imm12),
+    : RVInstI<funct3, OPC_CUSTOM_0, (outs GPR:$rd, GPR:$rs1_wb),
+              (ins GPRMem:$rs1, simm12:$imm12),
               opcodestr, "$rd, (${rs1}), ${imm12}"> {
   let Constraints = "$rs1_wb = $rs1";
   let DecoderNamespace = "XCV";
 }
 
 class CVLoad_rr_inc<bits<7> funct7, bits<3> funct3, string opcodestr>
-    : RVInstR<funct7, funct3, OPC_CUSTOM_1, (outs GPR:$rd, GPR:$rs1_wb), (ins GPRMem:$rs1, GPR:$rs2),
+    : RVInstR<funct7, funct3, OPC_CUSTOM_1, (outs GPR:$rd, GPR:$rs1_wb),
+              (ins GPRMem:$rs1, GPR:$rs2),
               opcodestr, "$rd, (${rs1}), ${rs2}"> {
   let Constraints = "$rs1_wb = $rs1";
   let DecoderNamespace = "XCV";
@@ -557,9 +557,9 @@ class CVLoad_rr<bits<7> funct7, bits<3> funct3, string opcodestr>
   let Inst{11-7} = rd;
   let DecoderNamespace = "XCV";
 }
+} // hasSideEffects = 0, mayLoad = 1, mayStore = 0
 
-let Predicates = [HasVendorXCVmem, IsRV32], hasSideEffects = 0, mayLoad = 1,
-    mayStore = 0 in {
+let Predicates = [HasVendorXCVmem, IsRV32] in {
   // Register-Immediate load with post-increment
   def CV_LB_ri_inc  : CVLoad_ri_inc<0b000, "cv.lb">;
   def CV_LBU_ri_inc : CVLoad_ri_inc<0b100, "cv.lbu">;
@@ -582,6 +582,7 @@ let Predicates = [HasVendorXCVmem, IsRV32], hasSideEffects = 0, mayLoad = 1,
   def CV_LW_rr  : CVLoad_rr<0b0000110, 0b011, "cv.lw">;
 }
 
+let hasSideEffects = 0, mayLoad = 0, mayStore = 1 in {
 class CVStore_ri_inc<bits<3> funct3, string opcodestr>
     : RVInstS<funct3, OPC_CUSTOM_1, (outs GPR:$rs1_wb),
               (ins GPR:$rs2, GPR:$rs1, simm12:$imm12),
@@ -622,9 +623,9 @@ class CVStore_rr<bits<3> funct3, bits<7> funct7, string opcodestr>
   let Inst{6-0} = OPC_CUSTOM_1.Value;
   let DecoderNamespace = "XCV";
 }
+} // hasSideEffects = 0, mayLoad = 0, mayStore = 1
 
-let Predicates = [HasVendorXCVmem, IsRV32], hasSideEffects = 0, mayLoad = 0,
-    mayStore = 1  in {
+let Predicates = [HasVendorXCVmem, IsRV32] in {
   // Register-Immediate store with post-increment
   def CV_SB_ri_inc : CVStore_ri_inc<0b000, "cv.sb">;
   def CV_SH_ri_inc : CVStore_ri_inc<0b001, "cv.sh">;

lenary

Some nits - I think you can maybe push things higher in the classes in some places, and from lets to inside the classes in others. I likely haven't captured everywhere this is possible.

I'm only really in favour of this for consistency, I do think it means you need to look at several different files to work out if you marked your instructions correctly (or just run tablegen to get the elaborated output), and as with CV_INSERTR there's always functions which you end up annotating anyway.

llvm/lib/Target/RISCV/RISCVInstrInfoXCV.td

lenary · 2025-03-11T04:09:09Z

llvm/lib/Target/RISCV/RISCVInstrInfoXCV.td

@@ -89,6 +91,7 @@ class CVInstMacMulN<bits<2> funct2, bits<3> funct3, dag outs, dag ins,
  let DecoderNamespace = "XCV";
 }

+let hasSideEffects = 0, mayLoad = 0, mayStore = 0 in {


These can, I think, go inside CVInstMacMulN

I didn't do that because I thought CVInstMacMulN was more like a format class like RISCVInstrFormats.td rather than a class in RISCVInstrInfo.td. So the flags would go on its derived classes if I keep the usual separation.

Though I was inconsistent about this reasoning for CVInstBitManipRII.

llvm/lib/Target/RISCV/RISCVInstrInfoXCV.td

… BranchCC_rri. NFC This is consistent with the isBranch and isTerminator flags already in the class. Addresses feedback given on llvm#130714 where I did the same inconsistent split in RISCVInstrInfoXCV.td.

topperc · 2025-03-11T05:58:39Z

I'm only really in favour of this for consistency, I do think it means you need to look at several different files to work out if you marked your instructions correctly (or just run tablegen to get the elaborated output), and as with CV_INSERTR there's always functions which you end up annotating anyway.

I do wonder if these flags are providing any value. Tablegen can infer them from single instruction patterns in many cases.

… BranchCC_rri. NFC (#130721) This is consistent with the isBranch and isTerminator flags already in the class. Addresses feedback given on #130714 where I copied the inconsistent split into RISCVInstrInfoXCV.td.

lenary · 2025-03-11T16:41:36Z

I do wonder if these flags are providing any value. Tablegen can infer them from single instruction patterns in many cases.

While I partially agree, it does depend on how we end up doing patterns - if they're in separate Pats, then tablegen does not infer this stuff. I think that we'll always end up with a few instructions that need the manual annotation because they don't have a pattern in the same way we'll always end up with a few instructions that don't need/justify their own individual class to put the annotations in.

topperc · 2025-03-11T16:47:59Z

if they're in separate Pats, then tablegen does not infer this stuff.

I think TableGen uses all of the patterns not just the one in the instruction.

[RISCV] Sink hasSideEffects, mayLoad, mayStore from defs to classes i…

3205636

…n RISCVInstrInfo.td. NFC This is consistent with how RISCVInstrInfo.td is generally structured.

topperc requested review from lenary, ChunyuLiao, melonedo, realqhc and wangpc-pp March 11, 2025 03:56

llvmbot added the backend:RISC-V label Mar 11, 2025

lenary approved these changes Mar 11, 2025

View reviewed changes

topperc mentioned this pull request Mar 11, 2025

[RISCV] Move let statement for hasSideEffects, mayLoad, mayStore into BranchCC_rri. NFC #130721

Merged

fixup! Address review comments

61031d4

topperc changed the title ~~[RISCV] Sink hasSideEffects, mayLoad, mayStore from defs to classes in RISCVInstrInfo.td. NFC~~ [RISCV] Sink hasSideEffects, mayLoad, mayStore from defs to classes in RISCVInstrInfoXCV.td. NFC Mar 11, 2025

topperc merged commit 3464766 into llvm:main Mar 11, 2025
11 checks passed

topperc deleted the pr/corev-flags branch March 11, 2025 16:47

topperc mentioned this pull request Mar 12, 2025

[RISCV] Add Qualcomm uC Xqcibi (Branch Immediate) extension #130779

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[RISCV] Sink hasSideEffects, mayLoad, mayStore from defs to classes in RISCVInstrInfoXCV.td. NFC #130714

[RISCV] Sink hasSideEffects, mayLoad, mayStore from defs to classes in RISCVInstrInfoXCV.td. NFC #130714

Uh oh!

topperc commented Mar 11, 2025

Uh oh!

llvmbot commented Mar 11, 2025

Uh oh!

lenary left a comment

Uh oh!

Uh oh!

lenary Mar 11, 2025

Uh oh!

topperc Mar 11, 2025 •

edited

Loading

Uh oh!

Uh oh!

topperc commented Mar 11, 2025

Uh oh!

Uh oh!

lenary commented Mar 11, 2025

Uh oh!

topperc commented Mar 11, 2025

Uh oh!

Uh oh!

[RISCV] Sink hasSideEffects, mayLoad, mayStore from defs to classes in RISCVInstrInfoXCV.td. NFC #130714

[RISCV] Sink hasSideEffects, mayLoad, mayStore from defs to classes in RISCVInstrInfoXCV.td. NFC #130714

Uh oh!

Conversation

topperc commented Mar 11, 2025

Uh oh!

llvmbot commented Mar 11, 2025

Uh oh!

lenary left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lenary Mar 11, 2025

Choose a reason for hiding this comment

Uh oh!

topperc Mar 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

topperc commented Mar 11, 2025

Uh oh!

Uh oh!

lenary commented Mar 11, 2025

Uh oh!

topperc commented Mar 11, 2025

Uh oh!

Uh oh!

topperc Mar 11, 2025 •

edited

Loading