[RISCV] Compress instructions based on function features

[simoncook]

Summary

When running under LTO, it is common to not specify the architecture
spec, which is used for setting up the target machine, and instead rely
on features specified in each function to generate the correct
instructions.

This works for the code generator, but the RISC-V backend uses the
AsmPrinter to do instruction compression, which does not see these
features but instead uses a MCSubtargetInfo object to see whether
compression is enabled. Since this is configured based on the
TargetMachine at startup, it will result in compressed instructions not
being emitted when it has not been given the 'c' TargetFeature, but the
function has it.

This changes the RISCVAsmPrinter to re-initialize the STI feature set
based on the current MachineFunction, such that compressed instructions
are now correctly emitted regardless of the method used to enable them.

[apazos]

Thanks for catching this issue, Simon. Alex, will this be cherry-picked to the branch?

[apazos]

llvm/test/CodeGen/RISCV/compress2.ll

6 ↗

(On Diff #240139)

Maybe a more meaningful name is compress-attribute.ll?

[luismarques]

Copying the the STI isn't quite trivial, and we're only using a small subset of what it provides, in compressInst. Can't this be done more tightly?

compressInst calls RISCVValidateMCOperand, which checks things like STI.getTargetTriple().isArch64Bit(). You said that "under LTO it is common to not specify the architecture". If that includes not properly initializing the STI target triple then those checks will fail in some circumstances. Please check also the status of the other members of the STI, in case the validation ends up using them in the future.

[lenary]

@simoncook Thank you for the patch! It's looking good.

@apazos I've had a brief chat with Alex about this. We do not intend to backport this change to 10.0, especially as LTO is not yet enabled for the RISC-V backend.

[simoncook]

In D73339#1864275, @luismarques wrote:

Copying the the STI isn't quite trivial, and we're only using a small subset of what it provides, in compressInst. Can't this be done more tightly?

compressInst calls RISCVValidateMCOperand, which checks things like STI.getTargetTriple().isArch64Bit(). You said that "under LTO it is common to not specify the architecture". If that includes not properly initializing the STI target triple then those checks will fail in some circumstances. Please check also the status of the other members of the STI, in case the validation ends up using them in the future.

I think isArch64Bit() will always be correct since we have to specify the triple, but I see your point with other features, I can imagine if we call llc/use LTO with +c and then have a function that enables one of the floating point flags, I'll look into and test that one.

As for testing this, it feels a shame to have to duplicate testcases like I have with compress.ll to cover both cases. Does it make sense to extend the ForceFunctionAttrsPass to add target-features to all functions, in which case we can have just one test case and check both ways of having attributes set? (unless anyone knows any other way we can minimize test duplication?)

[luismarques]

In D73339#1869058, @simoncook wrote:

As for testing this, it feels a shame to have to duplicate testcases like I have with compress.ll to cover both cases. Does it make sense to extend the ForceFunctionAttrsPass to add target-features to all functions, in which case we can have just one test case and check both ways of having attributes set? (unless anyone knows any other way we can minimize test duplication?)

It may be a bit of a hacky solution, but how about this? You keep one test file, add the attribute group references to the test functions, but don't add the attribute group definition to the source. Then in the RUN lines you cat the test source together with either an empty attribute group (the original tests) or the one with the target features.

[simoncook]

In D73339#1872048, @luismarques wrote:

It may be a bit of a hacky solution, but how about this? You keep one test file, add the attribute group references to the test functions, but don't add the attribute group definition to the source. Then in the RUN lines you cat the test source together with either an empty attribute group (the original tests) or the one with the target features.

That would work. Looking at other targets tests it looks like NVPTX uses this method to do something similar for adding metadata to tests (e.g. test/CodeGen/NTPTX/nvvm-relfect.ll), so I'll rebase this using this pattern to keep just one test.

[simoncook]

Add floating point test, combine compress tests into single test file.

[lenary]

I have one query at the moment:

llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp

160

Most backends seem to override AsmPrinter::runOnFunction entirely (the default implementation calls SetupMachineFunction), and avoid overriding SetupMachineFunction. Is there a particular reason you've chosen one vs the other? I note things like the ARM backend are pulling a SubtargetInfo out of the MachineFunction in their runOnFunction implementations, but I realise we end up needing a MCSubtargetInfo here - I presume that's the main reason?

[luismarques]

LGTM.

llvm/test/CodeGen/RISCV/compress-float.ll

2

Nitpick: one -> once

34

You're adding nounwind twice.

[simoncook]

llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp

160

No there isn't (or at least I can't remember there being a good reason), but thanks for pointing out this looks odd, going down the route of overriding runOnFunction as other backends do works as well, so I'm updating this to make this change in runOnFunction to match.

llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp

 
 #define DEBUG_TYPE "asm-printer"
 
 STATISTIC(RISCVNumInstrsCompressed,
           "Number of RISC-V Compressed instructions emitted");
 
 namespace {
 class RISCVAsmPrinter : public AsmPrinter {
+  const MCSubtargetInfo *STI;
+
 public:
   explicit RISCVAsmPrinter(TargetMachine &TM,
                            std::unique_ptr<MCStreamer> Streamer)
-      : AsmPrinter(TM, std::move(Streamer)) {}
+      : AsmPrinter(TM, std::move(Streamer)), STI(TM.getMCSubtargetInfo()) {}
 
   StringRef getPassName() const override { return "RISCV Assembly Printer"; }
 
+  void SetupMachineFunction(MachineFunction &MF) override;
+
   void EmitInstruction(const MachineInstr *MI) override;
 
   bool PrintAsmOperand(const MachineInstr *MI, unsigned OpNo,
                        const char *ExtraCode, raw_ostream &OS) override;
   bool PrintAsmMemoryOperand(const MachineInstr *MI, unsigned OpNo,
                              const char *ExtraCode, raw_ostream &OS) override;
 
   void EmitToStreamer(MCStreamer &S, const MCInst &Inst);
   bool emitPseudoExpansionLowering(MCStreamer &OutStreamer,
                                    const MachineInstr *MI);
 
   // Wrapper needed for tblgenned pseudo lowering.
   bool lowerOperand(const MachineOperand &MO, MCOperand &MCOp) const {
     return LowerRISCVMachineOperandToMCOperand(MO, MCOp, *this);
   }
 };
 }
 
 #define GEN_COMPRESS_INSTR
 #include "RISCVGenCompressInstEmitter.inc"
 void RISCVAsmPrinter::EmitToStreamer(MCStreamer &S, const MCInst &Inst) {
   MCInst CInst;
-  bool Res = compressInst(CInst, Inst, *TM.getMCSubtargetInfo(),
-                          OutStreamer->getContext());
+  bool Res = compressInst(CInst, Inst, *STI, OutStreamer->getContext());
   if (Res)
     ++RISCVNumInstrsCompressed;
   AsmPrinter::EmitToStreamer(*OutStreamer, Res ? CInst : Inst);
 }
 
 // Simple pseudo-instructions have their lowering (with expansion to real
 // instructions) auto-generated.
 #include "RISCVGenMCPseudoLowering.inc"
if (!ExtraCode) {
 
     OS << "0(" << RISCVInstPrinter::getRegisterName(MO.getReg()) << ")";
     return false;
   }
 
   return AsmPrinter::PrintAsmMemoryOperand(MI, OpNo, ExtraCode, OS);
 }
 
+void RISCVAsmPrinter::SetupMachineFunction(MachineFunction &MF) {
+  // Set the current MCSubtargetInfo to a copy which has the correct
+  // feature bits for the current MachineFunction
+  MCSubtargetInfo &NewSTI =
+    OutStreamer->getContext().getSubtargetCopy(*TM.getMCSubtargetInfo());
+  NewSTI.setFeatureBits(MF.getSubtarget().getFeatureBits());
+  STI = &NewSTI;
+
+  return AsmPrinter::SetupMachineFunction(MF);
+}
+
 // Force static initialization.
 extern "C" LLVM_EXTERNAL_VISIBILITY void LLVMInitializeRISCVAsmPrinter() {
   RegisterAsmPrinter<RISCVAsmPrinter> X(getTheRISCV32Target());
   RegisterAsmPrinter<RISCVAsmPrinter> Y(getTheRISCV64Target());
 }

llvm/test/CodeGen/RISCV/compress-float.ll

+; This test is designed to run three times, once with function attributes, once
+; with all target attributes added on the command line, and one with compress
+; added with the command line and float added via function attributes, all
+; three of these should result in the same output.
+;
+; RUN: cat %s > %t.tgtattr
+; RUN: echo 'attributes #0 = { nounwind }' >> %t.tgtattr
+; RUN: llc -mtriple=riscv32 -target-abi ilp32d -mattr=+c,+f,+d -filetype=obj \
+; RUN:   -disable-block-placement < %t.tgtattr \
+; RUN:   | llvm-objdump -d -triple=riscv32 -mattr=+c,+f,+d -M no-aliases - \
+; RUN:   | FileCheck -check-prefix=RV32IFDC %s
+;
+; RUN: cat %s > %t.fnattr
+; RUN: echo 'attributes #0 = { nounwind "target-features"="+c,+f,+d" }' >> %t.fnattr
+; RUN: llc -mtriple=riscv32 -target-abi ilp32d -filetype=obj \
+; RUN:   -disable-block-placement < %t.fnattr \
+; RUN:   | llvm-objdump -d -triple=riscv32 -mattr=+c,+f,+d -M no-aliases - \
+; RUN:   | FileCheck -check-prefix=RV32IFDC %s
+;
+; RUN: cat %s > %t.mixedattr
+; RUN: echo 'attributes #0 = { nounwind "target-features"="+f,+d" }' >> %t.mixedattr
+; RUN: llc -mtriple=riscv32 -target-abi ilp32d -mattr=+c -filetype=obj \
+; RUN:   -disable-block-placement < %t.mixedattr \
+; RUN:   | llvm-objdump -d -triple=riscv32 -mattr=+c,+f,+d -M no-aliases - \
+; RUN:   | FileCheck -check-prefix=RV32IFDC %s
+
+; This acts as a sanity check for the codegen instruction compression path,
+; verifying that the assembled file contains compressed instructions when
+; expected. Handling of the compressed ISA is implemented so the same
+; transformation patterns should be used whether compressing an input .s file or
+; compressing codegen output. This file contains sanity checks using
+; instructions which also require one of the floating point extensions.
+
+define float @float_load(float *%a) nounwind #0 {
+; RV32IFDC-LABEL: float_load:
+; RV32IFDC:         c.flw fa0, 0(a0)
+; RV32IFDC-NEXT:    c.jr ra
+  %1 = load volatile float, float* %a
+  ret float %1
+}
+
+define double @double_load(double *%a) nounwind #0 {
+; RV32IFDC-LABEL: double_load:
+; RV32IFDC:         c.fld fa0, 0(a0)
+; RV32IFDC-NEXT:    c.jr ra
+  %1 = load volatile double, double* %a
+  ret double %1
+}

llvm/test/CodeGen/RISCV/compress.ll

+; This test is designed to run twice, once with function attributes and once
+; with target attributes added on the command line.
+;
+; RUN: cat %s > %t.tgtattr
+; RUN: echo 'attributes #0 = { nounwind }' >> %t.tgtattr
 ; RUN: llc -mtriple=riscv32 -mattr=+c -filetype=obj \
-; RUN:   -disable-block-placement < %s \
+; RUN:   -disable-block-placement < %t.tgtattr \
+; RUN:   | llvm-objdump -d -triple=riscv32 -mattr=+c -M no-aliases - \
+; RUN:   | FileCheck -check-prefix=RV32IC %s
+;
+; RUN: cat %s > %t.fnattr
+; RUN: echo 'attributes #0 = { nounwind "target-features"="+c" }' >> %t.fnattr
+; RUN: llc -mtriple=riscv32 -filetype=obj \
+; RUN:   -disable-block-placement < %t.fnattr \
 ; RUN:   | llvm-objdump -d -triple=riscv32 -mattr=+c -M no-aliases - \
 ; RUN:   | FileCheck -check-prefix=RV32IC %s
 
 ; This acts as a sanity check for the codegen instruction compression path,
 ; verifying that the assembled file contains compressed instructions when
 ; expected. Handling of the compressed ISA is implemented so the same
 ; transformation patterns should be used whether compressing an input .s file or
 ; compressing codegen output. This file contains sanity checks to ensure that is
 ; working as expected. Particular care should be taken to test pseudo
 ; instructions.
 
 ; Note: TODOs in this file are only appropriate if they highlight a case where
 ; a generated instruction that can be compressed by an existing pattern isn't.
 ; It may be useful to have tests that indicate where better compression would be
 ; possible if alternative codegen choices were made, but they belong in a
 ; different test file.
 
-define i32 @simple_arith(i32 %a, i32 %b) nounwind {
+define i32 @simple_arith(i32 %a, i32 %b) nounwind #0 {
 ; RV32IC-LABEL: simple_arith:
 ; RV32IC:         addi a2, a0, 1
 ; RV32IC-NEXT:    c.andi a2, 11
 ; RV32IC-NEXT:    c.slli a2, 7
 ; RV32IC-NEXT:    c.srai a1, 9
 ; RV32IC-NEXT:    c.add a1, a2
 ; RV32IC-NEXT:    sub a0, a1, a0
 ; RV32IC-NEXT:    c.jr ra
   %1 = add i32 %a, 1
   %2 = and i32 %1, 11
   %3 = shl i32 %2, 7
   %4 = ashr i32 %b, 9
   %5 = add i32 %3, %4
   %6 = sub i32 %5, %a
   ret i32 %6
 }
 
-define i32 @select(i32 %a, i32 *%b) nounwind {
+define i32 @select(i32 %a, i32 *%b) nounwind #0 {
 ; RV32IC-LABEL: select:
 ; RV32IC:         c.lw a2, 0(a1)
 ; RV32IC-NEXT:    c.beqz a2, 4
 ; RV32IC-NEXT:    c.mv a0, a2
 ; RV32IC-NEXT:    c.lw a2, 0(a1)
 ; RV32IC-NEXT:    c.bnez a2, 4
 ; RV32IC-NEXT:    c.mv a0, a2
 ; RV32IC-NEXT:    c.lw a2, 0(a1)
; RV32IC-NEXT:    c.jr ra
 
   %val19 = load volatile i32, i32* %b
   %tst10 = icmp sle i32 %val18, %val19
   %val20 = select i1 %tst10, i32 %val18, i32 %val19
 
   ret i32 %val20
 }
 
-define i32 @pos_tiny() nounwind {
+define i32 @pos_tiny() nounwind #0 {
 ; RV32IC-LABEL: pos_tiny:
 ; RV32IC:         c.li a0, 18
 ; RV32IC-NEXT:    c.jr ra
   ret i32 18
 }
 
-define i32 @pos_i32() nounwind {
+define i32 @pos_i32() nounwind #0 {
 ; RV32IC-LABEL: pos_i32:
 ; RV32IC:         lui a0, 423811
 ; RV32IC-NEXT:    addi a0, a0, -1297
 ; RV32IC-NEXT:    c.jr ra
   ret i32 1735928559
 }
 
-define i32 @pos_i32_half_compressible() nounwind {
+define i32 @pos_i32_half_compressible() nounwind #0 {
 ; RV32IC-LABEL: pos_i32_half_compressible:
 ; RV32IC:         lui a0, 423810
 ; RV32IC-NEXT:    c.addi  a0, 28
 ; RV32IC-NEXT:    c.jr    ra
   ret i32 1735925788
 }
 
-
-define i32 @neg_tiny() nounwind {
+define i32 @neg_tiny() nounwind #0 {
 ; RV32IC-LABEL: neg_tiny:
 ; RV32IC:       c.li a0, -19
 ; RV32IC-NEXT:  c.jr ra
   ret i32 -19
 }
 
-define i32 @neg_i32() nounwind {
+define i32 @neg_i32() nounwind #0 {
 ; RV32IC-LABEL: neg_i32:
 ; RV32IC:       lui a0, 912092
 ; RV32IC-NEXT:  addi a0, a0, -273
 ; RV32IC-NEXT:  c.jr ra
   ret i32 -559038737
 }
 
-define i32 @pos_i32_hi20_only() nounwind {
+define i32 @pos_i32_hi20_only() nounwind #0 {
 ; RV32IC-LABEL: pos_i32_hi20_only:
 ; RV32IC:       c.lui a0, 16
 ; RV32IC-NEXT:  c.jr ra
   ret i32 65536
 }
 
-define i32 @neg_i32_hi20_only() nounwind {
+define i32 @neg_i32_hi20_only() nounwind #0 {
 ; RV32IC-LABEL: neg_i32_hi20_only:
 ; RV32IC:       c.lui a0, 1048560
 ; RV32IC-NEXT:  c.jr ra
   ret i32 -65536
 }