This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Fixed setting predicates for compressed instructions.
ClosedPublic

Authored by apazos on Jan 16 2018, 1:52 PM.

Details

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

Diff Detail

Repository
rL LLVM

Event Timeline

apazos created this revision.Jan 16 2018, 1:52 PM

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 accepted this revision.Jan 17 2018, 6:33 AM
This comment was removed by asb.
This revision is now accepted and ready to land.Jan 17 2018, 6:33 AM
asb requested changes to this revision.Jan 17 2018, 6:35 AM

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

This revision now requires changes to proceed.Jan 17 2018, 6:35 AM
asb added a comment.Jan 17 2018, 8:23 AM

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.

apazos updated this revision to Diff 130318.Jan 17 2018, 4:50 PM

Updated test cases, reusing the existing test files.

asb accepted this revision.Jan 18 2018, 4:01 AM

Thanks Ana, looks good to me.

test/MC/RISCV/rv64c-valid.s
59 ↗(On Diff #130318)

Unwanted whitespace

This revision is now accepted and ready to land.Jan 18 2018, 4:01 AM
apazos updated this revision to Diff 130452.Jan 18 2018, 10:50 AM

removed extra line in test case.

This revision was automatically updated to reflect the committed changes.