[RISCV] Fixed setting predicates for compressed instructions.

[apazos]

Summary

Fixed setting predicates for compressed instructions.
Some instructions were being generated with C extension
enabled only, without proper checks for the other
required extensions like F, D and 32 and 64-bit target checks.
Affected instructions:
C_FLD
C_FLW
C_LD
C_FSD
C_FSW
C_SD
C_JAL
C_ADDIW
C_SUBW
C_ADDW
C_FLDSP
C_FLWSP
C_LDSP
C_FSDSP
C_FSWSP
C_SDSP

[apazos]

Fixed setting target features.

Before we had:
let Predicates = [A] in {
def Inst : xxx, Requires[B];
}
This is not setting Predicates to A, B. It just takes A.

For example, c.addw should depend on C extension and 64bit support, but look at AsmMatcherEmitter auto generated header:
{ 1014 /* c.addw */, RISCV::C_ADDW, ConvertReg1_0Tie0__Reg1_1, Feature_HasStdExtC, { MCK_GPRC, MCK_GPRC }, },

You can also see the problem here:
echo 'c.addw x8, x9' |llvm-mc -triple=riscv64 -mattr=+c -show-encoding

c.addw  s0, s1                  # encoding: [0x25,0x9c]

echo 'c.addw x8, x9' |llvm-mc -triple=riscv32 -mattr=+c -show-encoding

c.addw  s0, s1                  # encoding: [0x25,0x9c]  --> should have failed.

There are different ways to fix it.
To avoid moving instructions around I just opted to redefining Predicates:
let Predicates = [A, B] in {
def Inst : xxx;
}

Now the auto-generated code looks correct:
{ 1014 /* c.addw */, RISCV::C_ADDW, ConvertReg1_0Tie0__Reg1_1, Feature_HasStdExtC|Feature_IsRV64, { MCK_GPRC, MCK_GPRC }, },

[asb]

This comment was removed by asb.

[asb]

Temporarily unaccepting - sorry, I accidentally added comments and acceptance for a different patch. Comments following shortly.

[asb]

Thanks for this - definitely a real problem that we want to fix.

Looking around, I see Mips, AMDGPU and Nios define a 'PredicateControl' class which they use to get more fine-grained control over predicates. I think this is more complex than what we require, and your fix of just explicitly overriding Predicates with a complete list is better for our use-case.

So I'm happy with the .td changes. For the test changes, could we extend the existing tests instead? e.g. modify test/MC/RISCV/rv32fc-valid.s to add new RUN lines for mattr with only +c, and new check lines for the error? So rv32fc-valid.s would look like:

# RUN: llvm-mc %s -triple=riscv32 -mattr=+c,+f -riscv-no-aliases -show-encoding \
# RUN:     | FileCheck -check-prefixes=CHECK,CHECK-INST %s
# RUN: llvm-mc -filetype=obj -triple=riscv32 -mattr=+c,+f < %s \
# RUN:     | llvm-objdump -mattr=+c,+f -riscv-no-aliases -d - \
# RUN:     | FileCheck -check-prefix=CHECK-INST %s
# RUN: not llvm-mc -triple riscv32 -mattr=+c < %s 2>&1 \
# RUN:     | FileCheck -check-prefix=CHECK-NOF %s


# CHECK-INST: c.flwsp  fs0, 252(sp)
# CHECK: encoding: [0x7e,0x74]
# CHECK-NOF: :[[@LINE+1]]:1: error: instruction use requires an option to be enabled
c.flwsp  fs0, 252(sp)
# CHECK-INST: c.fswsp  fa7, 252(sp)
# CHECK: encoding: [0xc6,0xff]
# CHECK-NOF: :[[@LINE+1]]:1: error: instruction use requires an option to be enabled
c.fswsp  fa7, 252(sp)

# CHECK-INST: c.flw  fa3, 124(a5)
# CHECK: encoding: [0xf4,0x7f]
# CHECK-NOF: :[[@LINE+1]]:1: error: instruction use requires an option to be enabled
c.flw  fa3, 124(a5)
# CHECK-INST: c.fsw  fa2, 124(a1)
# CHECK: encoding: [0xf0,0xfd]
# CHECK-NOF: :[[@LINE+1]]:1: error: instruction use requires an option to be enabled
c.fsw  fa2, 124(a1)

[apazos]

Hi Alex, I kept the new test cases in a separate file because the existing tests check for valid operands, not for failures due to extensions.
But I can try to merge them.

[apazos]

Updated test cases, reusing the existing test files.

[asb]

Thanks Ana, looks good to me.

test/MC/RISCV/rv64c-valid.s

59 ↗

(On Diff #130318)

Unwanted whitespace

[apazos]

removed extra line in test case.

lib/Target/RISCV/RISCVInstrInfoC.td

def C_ADDI4SPN : RVInst16CIW<0b000, 0b00, (outs GPRC:$rd),
                              "c.addi4spn", "$rd, $rs1, $imm"> {
   bits<5> rs1;
   let Inst{12-11} = imm{5-4};
   let Inst{10-7} = imm{9-6};
   let Inst{6} = imm{2};
   let Inst{5} = imm{3};
 }
 
-def C_FLD  : CLoad_ri<0b001, "c.fld", FPR64C, uimm8_lsb000>,
-             Requires<[HasStdExtD]> {
+let Predicates = [HasStdExtC, HasStdExtD] in
+def C_FLD  : CLoad_ri<0b001, "c.fld", FPR64C, uimm8_lsb000> {
   bits<8> imm;
   let Inst{12-10} = imm{5-3};
   let Inst{6-5} = imm{7-6};
 }
 
 def C_LW : CLoad_ri<0b010, "c.lw", GPRC, uimm7_lsb00> {
   bits<7> imm;
   let Inst{12-10} = imm{5-3};
   let Inst{6} = imm{2};
   let Inst{5} = imm{6};
 }
 
-let DecoderNamespace = "RISCV32Only_" in
-def C_FLW  : CLoad_ri<0b011, "c.flw", FPR32C, uimm7_lsb00>,
-             Requires<[HasStdExtF, IsRV32]> {
+let DecoderNamespace = "RISCV32Only_",
+    Predicates = [HasStdExtC, HasStdExtF, IsRV32] in
+def C_FLW  : CLoad_ri<0b011, "c.flw", FPR32C, uimm7_lsb00> {
   bits<7> imm;
   let Inst{12-10} = imm{5-3};
   let Inst{6} = imm{2};
   let Inst{5} = imm{6};
 }
 
-def C_LD : CLoad_ri<0b011, "c.ld", GPRC, uimm8_lsb000>,
-           Requires<[IsRV64]> {
+let Predicates = [HasStdExtC, IsRV64] in
+def C_LD : CLoad_ri<0b011, "c.ld", GPRC, uimm8_lsb000> {
   bits<8> imm;
   let Inst{12-10} = imm{5-3};
   let Inst{6-5} = imm{7-6};
 }
 
-def C_FSD  : CStore_rri<0b101, "c.fsd", FPR64C, uimm8_lsb000>,
-             Requires<[HasStdExtD]> {
+let Predicates = [HasStdExtC, HasStdExtD] in
+def C_FSD  : CStore_rri<0b101, "c.fsd", FPR64C, uimm8_lsb000> {
   bits<8> imm;
   let Inst{12-10} = imm{5-3};
   let Inst{6-5} = imm{7-6};
 }
 
 def C_SW : CStore_rri<0b110, "c.sw", GPRC, uimm7_lsb00> {
   bits<7> imm;
   let Inst{12-10} = imm{5-3};
   let Inst{6} = imm{2};
   let Inst{5} = imm{6};
 }
 
-let DecoderNamespace = "RISCV32Only_" in
-def C_FSW  : CStore_rri<0b111, "c.fsw", FPR32C, uimm7_lsb00>,
-             Requires<[HasStdExtF, IsRV32]> {
+let DecoderNamespace = "RISCV32Only_",
+    Predicates = [HasStdExtC, HasStdExtF, IsRV32]  in
+def C_FSW  : CStore_rri<0b111, "c.fsw", FPR32C, uimm7_lsb00> {
   bits<7> imm;
   let Inst{12-10} = imm{5-3};
   let Inst{6} = imm{2};
   let Inst{5} = imm{6};
 }
 
-def C_SD : CStore_rri<0b111, "c.sd", GPRC, uimm8_lsb000>,
-           Requires<[IsRV64]> {
+let Predicates = [HasStdExtC, IsRV64] in
+def C_SD : CStore_rri<0b111, "c.sd", GPRC, uimm8_lsb000> {
   bits<8> imm;
   let Inst{12-10} = imm{5-3};
   let Inst{6-5} = imm{7-6};
 }
 
 let rd = 0, imm = 0, hasSideEffects = 0, mayLoad = 0, mayStore = 0 in
 def C_NOP : RVInst16CI<0b000, 0b01, (outs), (ins), "c.nop", "">;
 
 let hasSideEffects = 0, mayLoad = 0, mayStore = 0 in
 def C_ADDI : RVInst16CI<0b000, 0b01, (outs GPRNoX0:$rd_wb),
                         (ins GPRNoX0:$rd, simm6:$imm),
                         "c.addi", "$rd, $imm"> {
   let Constraints = "$rd = $rd_wb";
   let Inst{6-2} = imm{4-0};
 }
 
 let hasSideEffects = 0, mayLoad = 0, mayStore = 0, isCall = 1,
-    DecoderNamespace = "RISCV32Only_", Defs = [X1] in
+    DecoderNamespace = "RISCV32Only_", Defs = [X1],
+    Predicates = [HasStdExtC, IsRV32]  in
 def C_JAL : RVInst16CJ<0b001, 0b01, (outs), (ins simm12_lsb0:$offset),
-                       "c.jal", "$offset">,
-            Requires<[IsRV32]>;
+                       "c.jal", "$offset">;
 
-let hasSideEffects = 0, mayLoad = 0, mayStore = 0 in
+let hasSideEffects = 0, mayLoad = 0, mayStore = 0,
+    Predicates = [HasStdExtC, IsRV64] in
 def C_ADDIW : RVInst16CI<0b001, 0b01, (outs GPRNoX0:$rd_wb),
                          (ins GPRNoX0:$rd, simm6:$imm),
-                         "c.addiw", "$rd, $imm">,
-              Requires<[IsRV64]> {
+                         "c.addiw", "$rd, $imm"> {
   let Constraints = "$rd = $rd_wb";
   let Inst{6-2} = imm{4-0};
 }
 
 let hasSideEffects = 0, mayLoad = 0, mayStore = 0 in
 def C_LI : RVInst16CI<0b010, 0b01, (outs GPRNoX0:$rd), (ins simm6:$imm),
                       "c.li", "$rd, $imm"> {
   let Inst{6-2} = imm{4-0};
def C_ANDI : RVInst16CB<0b100, 0b01, (outs GPRC:$rs1_wb), (ins GPRC:$rs1, simm6:$imm),
   let Inst{6-2} = imm{4-0};
 }
 
 def C_SUB  : CS_ALU<0b00, "c.sub", GPRC, 0>;
 def C_XOR  : CS_ALU<0b01, "c.xor", GPRC, 0>;
 def C_OR   : CS_ALU<0b10, "c.or" , GPRC, 0>;
 def C_AND  : CS_ALU<0b11, "c.and", GPRC, 0>;
 
-def C_SUBW : CS_ALU<0b00, "c.subw", GPRC, 1>, Requires<[IsRV64]>;
-def C_ADDW : CS_ALU<0b01, "c.addw", GPRC, 1>, Requires<[IsRV64]>;
+let Predicates = [HasStdExtC, IsRV64] in {
+def C_SUBW : CS_ALU<0b00, "c.subw", GPRC, 1>;
+def C_ADDW : CS_ALU<0b01, "c.addw", GPRC, 1>;
+}
 
 let hasSideEffects = 0, mayLoad = 0, mayStore = 0 in
 def C_J : RVInst16CJ<0b101, 0b01, (outs), (ins simm12_lsb0:$offset),
                      "c.j", "$offset"> {
   let isBranch = 1;
   let isTerminator=1;
   let isBarrier=1;
 }
 
 def C_BEQZ : Bcz<0b110, "c.beqz",  seteq, GPRC>;
 def C_BNEZ : Bcz<0b111, "c.bnez",  setne, GPRC>;
 
 let hasSideEffects = 0, mayLoad = 0, mayStore = 0 in
 def C_SLLI : RVInst16CI<0b000, 0b10, (outs GPRNoX0:$rd_wb),
                         (ins GPRNoX0:$rd, uimmlog2xlennonzero:$imm),
                         "c.slli" ,"$rd, $imm"> {
   let Constraints = "$rd = $rd_wb";
   let Inst{6-2} = imm{4-0};
 }
 
-def C_FLDSP  : CStackLoad<0b001, "c.fldsp", FPR64, uimm9_lsb000>,
-               Requires<[HasStdExtD]> {
+let Predicates = [HasStdExtC, HasStdExtD] in
+def C_FLDSP  : CStackLoad<0b001, "c.fldsp", FPR64, uimm9_lsb000> {
   let Inst{6-5} = imm{4-3};
   let Inst{4-2} = imm{8-6};
 }
 
 def C_LWSP : CStackLoad<0b010, "c.lwsp", GPRNoX0, uimm8_lsb00> {
   let Inst{6-4} = imm{4-2};
   let Inst{3-2} = imm{7-6};
 }
 
-let DecoderNamespace = "RISCV32Only_" in
-def C_FLWSP  : CStackLoad<0b011, "c.flwsp", FPR32, uimm8_lsb00>,
-               Requires<[HasStdExtF, IsRV32]> {
+let DecoderNamespace = "RISCV32Only_",
+    Predicates = [HasStdExtC, HasStdExtF, IsRV32] in
+def C_FLWSP  : CStackLoad<0b011, "c.flwsp", FPR32, uimm8_lsb00> {
   let Inst{6-4} = imm{4-2};
   let Inst{3-2} = imm{7-6};
 }
 
-def C_LDSP : CStackLoad<0b011, "c.ldsp", GPRNoX0, uimm9_lsb000>,
-             Requires<[IsRV64]> {
+let Predicates = [HasStdExtC, IsRV64] in
+def C_LDSP : CStackLoad<0b011, "c.ldsp", GPRNoX0, uimm9_lsb000> {
   let Inst{6-5} = imm{4-3};
   let Inst{4-2} = imm{8-6};
 }
 
 let hasSideEffects = 0, mayLoad = 0, mayStore = 0 in
 def C_JR : RVInst16CR<0b1000, 0b10, (outs), (ins GPRNoX0:$rs1),
                       "c.jr", "$rs1"> {
   let isBranch = 1;

 
 let hasSideEffects = 0, mayLoad = 0, mayStore = 0 in
 def C_ADD : RVInst16CR<0b1001, 0b10, (outs GPRNoX0:$rs1_wb),
                        (ins GPRNoX0:$rs1, GPRNoX0:$rs2),
                        "c.add", "$rs1, $rs2"> {
   let Constraints = "$rs1 = $rs1_wb";
 }
 
-def C_FSDSP  : CStackStore<0b101, "c.fsdsp", FPR64, uimm9_lsb000>,
-               Requires<[HasStdExtD]> {
+let Predicates = [HasStdExtC, HasStdExtD] in
+def C_FSDSP  : CStackStore<0b101, "c.fsdsp", FPR64, uimm9_lsb000> {
   let Inst{12-10} = imm{5-3};
   let Inst{9-7}   = imm{8-6};
 }
 
 def C_SWSP : CStackStore<0b110, "c.swsp", GPR, uimm8_lsb00> {
   let Inst{12-9} = imm{5-2};
   let Inst{8-7}  = imm{7-6};
 }
 
-let DecoderNamespace = "RISCV32Only_" in
-def C_FSWSP  : CStackStore<0b111, "c.fswsp", FPR32, uimm8_lsb00>,
-               Requires<[HasStdExtF, IsRV32]> {
+let DecoderNamespace = "RISCV32Only_",
+    Predicates = [HasStdExtC, HasStdExtF, IsRV32] in
+def C_FSWSP  : CStackStore<0b111, "c.fswsp", FPR32, uimm8_lsb00> {
   let Inst{12-9} = imm{5-2};
   let Inst{8-7}  = imm{7-6};
 }
 
-def C_SDSP : CStackStore<0b111, "c.sdsp", GPR, uimm9_lsb000>,
-             Requires<[IsRV64]> {
+let Predicates = [HasStdExtC, IsRV64] in
+def C_SDSP : CStackStore<0b111, "c.sdsp", GPR, uimm9_lsb000> {
   let Inst{12-10} = imm{5-3};
   let Inst{9-7}   = imm{8-6};
 }
 
 } // Predicates = [HasStdExtC]

test/MC/RISCV/rv32c-ext-check1.s

+# RUN: llvm-mc -triple=riscv32 -mattr=+c,+f \
+# RUN: -riscv-no-aliases -show-encoding < %s \
+# RUN: | FileCheck -check-prefixes=CHECK-BITS,CHECK-INST %s
+# RUN: not llvm-mc -triple riscv64 -mattr=+c,+f \
+# RUN: -riscv-no-aliases -show-encoding < %s 2>&1 \
+# RUN: | FileCheck %s
+# RUN: not llvm-mc -triple riscv32 -mattr=+c \
+# RUN: -riscv-no-aliases -show-encoding < %s 2>&1 \
+# RUN: | FileCheck %s
+# RUN: not llvm-mc -triple riscv32 \
+# RUN: -riscv-no-aliases -show-encoding < %s 2>&1 \
+# RUN: | FileCheck %s
+
+# Instruction requires C and F extensions and is 32bit-only.
+# CHECK-INST: c.flw fs0, 124(s0)
+# CHECK-BITS: encoding: [0x60,0x7c]
+# CHECK:  error: instruction use requires an option to be enabled
+c.flw fs0, 124(s0)
+# CHECK-INST: c.fsw fs0, 124(s0)
+# CHECK-BITS: encoding: [0x60,0xfc]
+# CHECK: error: instruction use requires an option to be enabled
+c.fsw fs0, 124(s0)
+# CHECK-INST: c.flwsp fs0, 124(sp)
+# CHECK-BITS:  encoding: [0x76,0x74]
+# CHECK: error: instruction use requires an option to be enabled
+c.flwsp fs0, 124(sp)
+# CHECK-INST: c.fswsp fs0, 124(sp)
+# CHECK-BITS: encoding: [0xa2,0xfe]
+# CHECK: error: instruction use requires an option to be enabled
+c.fswsp fs0, 124(sp)
+

test/MC/RISCV/rv32c-ext-check2.s

+# RUN: llvm-mc -triple=riscv32 -mattr=+c \
+# RUN: -riscv-no-aliases -show-encoding < %s \
+# RUN: | FileCheck -check-prefixes=CHECK-BITS,CHECK-INST %s
+# RUN: not llvm-mc -triple riscv64 -mattr=+c \
+# RUN: -riscv-no-aliases -show-encoding < %s 2>&1 \
+# RUN: | FileCheck %s
+# RUN: not llvm-mc -triple riscv32 \
+# RUN: -riscv-no-aliases -show-encoding < %s 2>&1 \
+# RUN: | FileCheck %s
+
+# Instruction is 32-bit only.
+# CHECK-INST: c.jal 2046
+# CHECK-BITS: encoding:  [0xfd,0x2f]
+# CHECK:  error: instruction use requires an option to be enabled
+c.jal 2046

test/MC/RISCV/rv32c-rv64c-ext-check.s

+# RUN: llvm-mc -triple=riscv32 -mattr=+c,+d \
+# RUN: -riscv-no-aliases -show-encoding < %s \
+# RUN: | FileCheck -check-prefixes=CHECK-BITS,CHECK-INST %s
+# RUN: llvm-mc -triple=riscv64 -mattr=+c,+d \
+# RUN: -riscv-no-aliases -show-encoding < %s \
+# RUN: | FileCheck -check-prefixes=CHECK-BITS,CHECK-INST %s
+# RUN: not llvm-mc -triple riscv32 -mattr=+c\
+# RUN: -riscv-no-aliases -show-encoding < %s 2>&1 \
+# RUN: | FileCheck %s
+# RUN: not llvm-mc -triple riscv64 -mattr=+c\
+# RUN: -riscv-no-aliases -show-encoding < %s 2>&1 \
+# RUN: | FileCheck %s
+# RUN: not llvm-mc -triple riscv32 \
+# RUN: -riscv-no-aliases -show-encoding < %s 2>&1 \
+# RUN: | FileCheck %s
+# RUN: not llvm-mc -triple riscv64 \
+# RUN: -riscv-no-aliases -show-encoding < %s 2>&1 \
+# RUN: | FileCheck %s
+
+# Instruction requires C and D extensions, is 32-bit and 64-bit.
+# CHECK-INST: c.fld fs0, 248(s0)
+# CHECK-BITS: encoding: [0x60,0x3c]
+# CHECK:  error: instruction use requires an option to be enabled
+c.fld fs0, 248(s0)
+# CHECK-INST: c.fsd fs0, 248(s0)
+# CHECK-BITS: encoding: [0x60,0xbc]
+# CHECK:  error: instruction use requires an option to be enabled
+c.fsd fs0, 248(s0)
+# CHECK2-INST: c.fldsp fs0, 64 (sp)
+# CHECK2-BITS: encoding: [0x06,0x24]
+# CHECK:  error: instruction use requires an option to be enabled
+c.fldsp fs0, 64 (sp)
+# CHECK-INST: c.fsdsp fs0, 64(sp)
+# CHECK-BITS: encoding:  [0xa2,0xa0]
+# CHECK:  error: instruction use requires an option to be enabled
+c.fsdsp fs0, 64 (sp)
+
+

test/MC/RISCV/rv64c-ext-check.s

+# RUN: llvm-mc -triple=riscv64 -mattr=+c \
+# RUN: -riscv-no-aliases -show-encoding < %s \
+# RUN: | FileCheck -check-prefixes=CHECK-BITS,CHECK-INST %s
+
+# RUN: not llvm-mc -triple riscv32 -mattr=+c \
+# RUN: -riscv-no-aliases -show-encoding < %s 2>&1 \
+# RUN: | FileCheck %s
+# RUN: not llvm-mc -triple riscv32 \
+# RUN: -riscv-no-aliases -show-encoding < %s 2>&1 \
+# RUN: | FileCheck %s
+# RUN: not llvm-mc -triple riscv64 \
+# RUN: -riscv-no-aliases -show-encoding < %s 2>&1 \
+# RUN: | FileCheck %s
+
+# Instruction is 64bit-only.
+# CHECK-INST: c.ld s1, 248(s0)
+# CHECK-BITS: encoding:
+# CHECK:  error: instruction use requires an option to be enabled
+c.ld s1, 248(s0)
+# CHECK-INST:c.sd s1, 248(s0)
+# CHECK-BITS: encoding:
+# CHECK:  error: instruction use requires an option to be enabled
+c.sd s1, 248(s0)
+# CHECK-INST: c.ldsp s1, 248(sp)
+# CHECK-BITS: encoding:
+# CHECK:  error: instruction use requires an option to be enabled
+c.ldsp s1, 248(sp)
+# CHECK-INST: c.sdsp s1, 248(sp)
+# CHECK-BITS: encoding:
+# CHECK:  error: instruction use requires an option to be enabled
+c.sdsp s1, 248(sp)
+# CHECK-INST: c.addiw s0, 31
+# CHECK-BITS: encoding:
+# CHECK:  error: instruction use requires an option to be enabled
+c.addiw s0, 31
+# CHECK-INST: c.addw s0, s1
+# CHECK-BITS: encoding:
+# CHECK:  error: instruction use requires an option to be enabled
+c.addw s0, s1
+# CHECK-INST: c.subw s0, s1
+# CHECK-BITS: encoding:
+# CHECK:  error: instruction use requires an option to be enabled
+c.subw s0, s1
+