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
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
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 }, },
Temporarily unaccepting - sorry, I accidentally added comments and acceptance for a different patch. Comments following shortly.
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)
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.
Thanks Ana, looks good to me.
test/MC/RISCV/rv64c-valid.s | ||
---|---|---|
59 ↗ | (On Diff #130318) | Unwanted whitespace |