[RISCV] Support clang -fpatchable-function-entry && GNU function attribute 'patchable_function_entry'

[MaskRay]

Summary

Similar to D72215 (AArch64) and D72220 (x86).

% clang -target riscv32 -march=rv64g -c -fpatchable-function-entry=2 a.c && llvm-objdump -dr a.o
...
0000000000000000 <main>:
       0: 13 00 00 00   nop
       4: 13 00 00 00   nop

% clang -target riscv32 -march=rv64gc -c -fpatchable-function-entry=2 a.c && llvm-objdump -dr a.o
...
00000002 <main>:
       2: 01 00         nop
       4: 01 00         nop

Recently the mainline kernel started to use -fpatchable-function-entry=8 for riscv (https://git.kernel.org/linus/afc76b8b80112189b6f11e67e19cf58301944814).

[luismarques]

Overall LGTM.
I just don't understand what you mean with "1-byte NOPs" in the patchable prefix case. Regular NOPs are emitted. Please clarify the comment/patch as needed.

[MaskRay]

fix comment

[jrtc27]

llvm/lib/Target/RISCV/RISCVInstrInfo.cpp

53

I will forever wonder why TII didn't make it MCInst getNoop()...

llvm/lib/Target/RISCV/RISCVMCInstLower.cpp

247

true, surely?

llvm/test/CodeGen/RISCV/patchable-function-entry.ll

2–4

Please match the style of the other tests:

• update_llc_test_checks.py
• RV32I/RV32IC/RV64I/RV64IC

6–10

I don't think this is particularly useful if we've checked the assembly output (and it stops us using update_llc_test_checks.py), we know nops get emitted correctly, that's tested in various other places.

[luismarques]

llvm/test/CodeGen/RISCV/patchable-function-entry.ll

2–4

Please match the style of the other tests:

• update_llc_test_checks.py

I was going to comment on that but I was unsure it applied here, given that this test was running more than just llc. The other RISC-V CodeGen tests we have with objdump don't use the update script IIRC. (And these manual tests were neatly written and much more condensed that the sprawling output of the auto updated checks). But I agree that in general using the script is the way to go for the RISC-V tests.

[aaron.ballman]

The attribute bits look fine to me aside from the documentation request.

clang/include/clang/Basic/Attr.td

738

We should probably add some documentation about what targets this attribute is supported on.

[jrtc27]

llvm/test/CodeGen/RISCV/patchable-function-entry.ll

2–4

Yeah, though as you'll see in my comment below I don't think using llvm-objdump adds any value, at which point UTC works just fine.

[MaskRay]

llvm/lib/Target/RISCV/RISCVMCInstLower.cpp

247

false.

We want to call EmitToStreamer(*OutStreamer, TmpInst);

If a pseudo instruction is not handled here, in MCAsmStreamer we will see its comment (AsmString).

llvm/test/CodeGen/RISCV/patchable-function-entry.ll

2–4

I do not think we can use update_llc_test_checks.py. We should test __patchable_function_entries which can be scrubbed by update_llc_test_checks.py.

The test is written in such a way with low maintenance: it will very unlikely change due to codegen differences. (The one ; CHECK-NEXT: addi sp, sp, -16 below is the only exception.)

Testing more variants seems wasteful.

6–10

objdump -d/llvm-objdump -d does not print c.nop => it is an important detail. Without it we cannot make sure the correct number of bytes is used.

[jrtc27]

llvm/test/CodeGen/RISCV/patchable-function-entry.ll

2–4

Testing more variants is not wasteful if you can unify the compressed checking RUN lines below with these ones. There would only be 4 separate RUN lines in total, one for each, checking the assembly output (without aliases).

6–10

Then use -riscv-no-aliases for the assembly-printing llc above, you don't need to go through assembling and dumping

[MaskRay]

Mention supported targets in clang/include/clang/Basic/AttrDocs.td

Use 4 llc RUN lines and --riscv-no-aliases

[MaskRay]

llvm/lib/Target/RISCV/RISCVInstrInfo.cpp

53

Let me do a refactoring before this patch?

[jrtc27]

llvm/lib/Target/RISCV/RISCVInstrInfo.cpp

53

Definitely won't complain if you're volunteering to fix it to be a saner interface :)

llvm/lib/Target/RISCV/RISCVMCInstLower.cpp

247

Ok, yes, I agree now, worked out what's going on

llvm/test/CodeGen/RISCV/patchable-function-entry.ll

2–4

Ok now please use RV32/RV64 rather than bare 32/64 to be more consistent with existing tests

25

Is there a benefit from treating these like labels with CHECK-LABEL?

[MaskRay]

Change check prefixes from 32/64 to RV32/RV64

Rebase and use MCInst getNop

llvm/test/CodeGen/RISCV/patchable-function-entry.ll

25

I don't find a difference regarding the diagnostic.

The <stdin>:29:2: note: possible intended match here\n .section __patchable_function_entries,"awo",@progbits,f1 lines do not change. So -LABEL: does not improve diagnostic.

clang/include/clang/Basic/Attr.td

def XRayLogArgs : InheritableAttr {
   // to include any C++ implicit this parameter.  Thus, we just store it and
   // use it as an unsigned that never needs adjustment.
   let Args = [UnsignedArgument<"ArgumentCount">];
   let Documentation = [XRayDocs];
 }
 
 def PatchableFunctionEntry
     : InheritableAttr,
-      TargetSpecificAttr<TargetArch<["aarch64", "aarch64_be", "x86", "x86_64"]>> {
+      TargetSpecificAttr<TargetArch<
+          ["aarch64", "aarch64_be", "riscv32", "riscv64", "x86", "x86_64"]>> {
   let Spellings = [GCC<"patchable_function_entry">];
   let Subjects = SubjectList<[Function, ObjCMethod]>;
   let Args = [UnsignedArgument<"Count">, DefaultIntArgument<"Offset", 0>];
   let Documentation = [PatchableFunctionEntryDocs];
 }
 
 def TLSModel : InheritableAttr {
   let Spellings = [GCC<"tls_model">];
   let Subjects = SubjectList<[TLSVar], ErrorDiag>;
   let Args = [StringArgument<"Model">];
   let Documentation = [TLSModelDocs];
 }

clang/lib/Driver/ToolChains/Clang.cpp

if (D.getVFS().exists(Filename))
       CmdArgs.push_back(Args.MakeArgString("-fprofile-list=" + Filename));
     else
       D.Diag(clang::diag::err_drv_no_such_file) << Filename;
   }
 
   if (Arg *A = Args.getLastArg(options::OPT_fpatchable_function_entry_EQ)) {
     StringRef S0 = A->getValue(), S = S0;
     unsigned Size, Offset = 0;
-    if (!Triple.isAArch64() && Triple.getArch() != llvm::Triple::x86 &&
-        Triple.getArch() != llvm::Triple::x86_64)
+    if (!Triple.isAArch64() && !Triple.isRISCV() && !Triple.isX86())
       D.Diag(diag::err_drv_unsupported_opt_for_target)
           << A->getAsString(Args) << TripleStr;
     else if (S.consumeInteger(10, Size) ||
              (!S.empty() && (!S.consume_front(",") ||
                              S.consumeInteger(10, Offset) || !S.empty())))
       D.Diag(diag::err_drv_invalid_argument_to_option)
           << S0 << A->getOption().getName();
     else if (Size < Offset)

clang/test/Driver/fpatchable-function-entry.c

 // RUN: %clang -target i386 %s -fpatchable-function-entry=1 -c -### 2>&1 | FileCheck %s
 // RUN: %clang -target x86_64 %s -fpatchable-function-entry=1 -c -### 2>&1 | FileCheck %s
 // RUN: %clang -target aarch64 %s -fpatchable-function-entry=1 -c -### 2>&1 | FileCheck %s
 // RUN: %clang -target aarch64 %s -fpatchable-function-entry=1,0 -c -### 2>&1 | FileCheck %s
+// RUN: %clang -target riscv32 %s -fpatchable-function-entry=1,0 -c -### 2>&1 | FileCheck %s
+// RUN: %clang -target riscv64 %s -fpatchable-function-entry=1,0 -c -### 2>&1 | FileCheck %s
 // CHECK: "-fpatchable-function-entry=1"
 
 // RUN: %clang -target aarch64 -fsyntax-only %s -fpatchable-function-entry=1,1 -c -### 2>&1 | FileCheck --check-prefix=11 %s
 // 11: "-fpatchable-function-entry=1" "-fpatchable-function-entry-offset=1"
 // RUN: %clang -target aarch64 -fsyntax-only %s -fpatchable-function-entry=2,1 -c -### 2>&1 | FileCheck --check-prefix=21 %s
 // 21: "-fpatchable-function-entry=2" "-fpatchable-function-entry-offset=1"
 
 // RUN: not %clang -target ppc64 -fsyntax-only %s -fpatchable-function-entry=1 2>&1 | FileCheck --check-prefix=TARGET %s
 // TARGET: error: unsupported option '-fpatchable-function-entry=1' for target 'ppc64'
 
 // RUN: not %clang -target x86_64 -fsyntax-only %s -fpatchable-function-entry=1,0, 2>&1 | FileCheck --check-prefix=EXCESS %s
 // EXCESS: error: invalid argument '1,0,' to -fpatchable-function-entry=
 
 // RUN: not %clang -target aarch64-linux -fsyntax-only %s -fxray-instrument -fpatchable-function-entry=1 2>&1 | FileCheck --check-prefix=XRAY %s
 // XRAY: error: invalid argument '-fxray-instrument' not allowed with '-fpatchable-function-entry='

clang/test/Sema/patchable-function-entry-attr.cpp

 // RUN: %clang_cc1 -triple aarch64 -fsyntax-only -verify=silence %s
 // RUN: %clang_cc1 -triple aarch64_be -fsyntax-only -verify=silence %s
 // RUN: %clang_cc1 -triple i386 -fsyntax-only -verify=silence %s
 // RUN: %clang_cc1 -triple x86_64 -fsyntax-only -verify=silence %s
+// RUN: %clang_cc1 -triple riscv32 -fsyntax-only -verify=silence %s
+// RUN: %clang_cc1 -triple riscv64 -fsyntax-only -verify=silence %s
 // RUN: %clang_cc1 -triple ppc64le -fsyntax-only -verify %s
 
 // silence-no-diagnostics
 
 // expected-warning@+1 {{unknown attribute 'patchable_function_entry' ignored}}
 [[gnu::patchable_function_entry(0)]] void f();

llvm/lib/Target/RISCV/RISCV.h

 class FunctionPass;
 class InstructionSelector;
 class MCInst;
 class MCOperand;
 class MachineInstr;
 class MachineOperand;
 class PassRegistry;
 
-void LowerRISCVMachineInstrToMCInst(const MachineInstr *MI, MCInst &OutMI,
-                                    const AsmPrinter &AP);
+bool lowerRISCVMachineInstrToMCInst(const MachineInstr *MI, MCInst &OutMI,
+                                    AsmPrinter &AP);
 bool LowerRISCVMachineOperandToMCOperand(const MachineOperand &MO,
                                          MCOperand &MCOp, const AsmPrinter &AP);
 
 FunctionPass *createRISCVISelDag(RISCVTargetMachine &TM);
 
 FunctionPass *createRISCVMergeBaseOffsetOptPass();
 void initializeRISCVMergeBaseOffsetOptPass(PassRegistry &);
 

llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp

 #include "RISCVGenMCPseudoLowering.inc"
 
 void RISCVAsmPrinter::emitInstruction(const MachineInstr *MI) {
   // Do any auto-generated pseudo lowerings.
   if (emitPseudoExpansionLowering(*OutStreamer, MI))
     return;
 
   MCInst TmpInst;
-  LowerRISCVMachineInstrToMCInst(MI, TmpInst, *this);
+  if (!lowerRISCVMachineInstrToMCInst(MI, TmpInst, *this))
     EmitToStreamer(*OutStreamer, TmpInst);
 }
 
 bool RISCVAsmPrinter::PrintAsmOperand(const MachineInstr *MI, unsigned OpNo,
                                       const char *ExtraCode, raw_ostream &OS) {
   // First try the generic code, which knows about modifiers like 'c' and 'n'.
   if (!AsmPrinter::PrintAsmOperand(MI, OpNo, ExtraCode, OS))
     return false;
 

llvm/lib/Target/RISCV/RISCVInstrInfo.h

 
 class RISCVSubtarget;
 
 class RISCVInstrInfo : public RISCVGenInstrInfo {
 
 public:
   explicit RISCVInstrInfo(RISCVSubtarget &STI);
 
+  void getNoop(MCInst &NopInst) const override;
+
   unsigned isLoadFromStackSlot(const MachineInstr &MI,
                                int &FrameIndex) const override;
   unsigned isStoreToStackSlot(const MachineInstr &MI,
                               int &FrameIndex) const override;
 
   void copyPhysReg(MachineBasicBlock &MBB, MachineBasicBlock::iterator MBBI,
                    const DebugLoc &DL, MCRegister DstReg, MCRegister SrcReg,
                    bool KillSrc) const override;

llvm/lib/Target/RISCV/RISCVInstrInfo.cpp

 #include "RISCVTargetMachine.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/Analysis/MemoryLocation.h"
 #include "llvm/CodeGen/MachineFunctionPass.h"
 #include "llvm/CodeGen/MachineInstrBuilder.h"
 #include "llvm/CodeGen/MachineRegisterInfo.h"
 #include "llvm/CodeGen/RegisterScavenging.h"
+#include "llvm/MC/MCInstBuilder.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/TargetRegistry.h"
 
 using namespace llvm;
 
 #define GEN_CHECK_COMPRESS_INSTR
 #include "RISCVGenCompressInstEmitter.inc"
 

 
 } // namespace RISCVVPseudosTable
 } // namespace llvm
 
 RISCVInstrInfo::RISCVInstrInfo(RISCVSubtarget &STI)
     : RISCVGenInstrInfo(RISCV::ADJCALLSTACKDOWN, RISCV::ADJCALLSTACKUP),
       STI(STI) {}
 
+void RISCVInstrInfo::getNoop(MCInst &NopInst) const {
+  if (STI.getFeatureBits()[RISCV::FeatureStdExtC])
+    NopInst = MCInstBuilder(RISCV::C_NOP);
+  else
+    NopInst = MCInstBuilder(RISCV::ADDI)
+                  .addReg(RISCV::X0)
+                  .addReg(RISCV::X0)
+                  .addImm(0);
+}
+
 unsigned RISCVInstrInfo::isLoadFromStackSlot(const MachineInstr &MI,
                                              int &FrameIndex) const {
   switch (MI.getOpcode()) {
   default:
     return 0;
   case RISCV::LB:
   case RISCV::LBU:
   case RISCV::LH:

llvm/lib/Target/RISCV/RISCVMCInstLower.cpp

static bool lowerRISCVVMachineInstrToMCInst(const MachineInstr *MI,
   // Unmasked pseudo instructions need to append dummy mask operand to
   // V instructions. All V instructions are modeled as the masked version.
   if (TSFlags & RISCVII::HasDummyMaskOpMask)
     OutMI.addOperand(MCOperand::createReg(RISCV::NoRegister));
 
   return true;
 }
 
-void llvm::LowerRISCVMachineInstrToMCInst(const MachineInstr *MI, MCInst &OutMI,
-                                          const AsmPrinter &AP) {
+bool llvm::lowerRISCVMachineInstrToMCInst(const MachineInstr *MI, MCInst &OutMI,
+                                          AsmPrinter &AP) {
   if (lowerRISCVVMachineInstrToMCInst(MI, OutMI))
-    return;
+    return false;
 
   OutMI.setOpcode(MI->getOpcode());
 
   for (const MachineOperand &MO : MI->operands()) {
     MCOperand MCOp;
     if (LowerRISCVMachineOperandToMCOperand(MO, MCOp, AP))
       OutMI.addOperand(MCOp);
   }
 
-  if (OutMI.getOpcode() == RISCV::PseudoReadVLENB) {
+  switch (OutMI.getOpcode()) {
+  case TargetOpcode::PATCHABLE_FUNCTION_ENTER: {
+    const Function &F = MI->getParent()->getParent()->getFunction();
+    if (F.hasFnAttribute("patchable-function-entry")) {
+      unsigned Num;
+      if (F.getFnAttribute("patchable-function-entry")
+              .getValueAsString()
+              .getAsInteger(10, Num))
+        return false;
+      AP.emitNops(Num);
+      return true;
+    }
+    break;
+  }
+  case RISCV::PseudoReadVLENB:
     OutMI.setOpcode(RISCV::CSRRS);
     OutMI.addOperand(MCOperand::createImm(
         RISCVSysReg::lookupSysRegByName("VLENB")->Encoding));
     OutMI.addOperand(MCOperand::createReg(RISCV::X0));
-    return;
-  }
-
-  if (OutMI.getOpcode() == RISCV::PseudoReadVL) {
+    break;
+  case RISCV::PseudoReadVL:
     OutMI.setOpcode(RISCV::CSRRS);
-    OutMI.addOperand(MCOperand::createImm(
-        RISCVSysReg::lookupSysRegByName("VL")->Encoding));
+    OutMI.addOperand(
+        MCOperand::createImm(RISCVSysReg::lookupSysRegByName("VL")->Encoding));
     OutMI.addOperand(MCOperand::createReg(RISCV::X0));
-    return;
+    break;
   }
+  return false;
 }

llvm/test/CodeGen/RISCV/patchable-function-entry.ll

+;; Test the function attribute "patchable-function-entry".
+; RUN: llc -mtriple=riscv32 < %s | FileCheck %s --check-prefixes=CHECK,32
+; RUN: llc -mtriple=riscv64 < %s | FileCheck %s --check-prefixes=CHECK,64
+
+; RUN: llc -filetype=obj -mtriple=riscv64 %s -o %t.o
+; RUN: llvm-objdump -d %t.o | FileCheck %s --check-prefix=NORVC
+; RUN: llc -filetype=obj -mtriple=riscv64 -mattr=+c %s -o %tc.o
+; RUN: llvm-objdump -d %tc.o | FileCheck %s --check-prefix=RVC
+
+; NORVC: 13 00 00 00  nop
+; RVC:   01 00        nop
+
+define void @f0() "patchable-function-entry"="0" {
+; CHECK-LABEL: f0:
+; CHECK-NEXT:  .Lfunc_begin0:
+; CHECK-NOT:     nop
+; CHECK:         ret
+; CHECK-NOT:   .section __patchable_function_entries
+  ret void
+}
+
+define void @f1() "patchable-function-entry"="1" {
+; CHECK-LABEL: f1:
+; CHECK-NEXT: .Lfunc_begin1:
+; CHECK:         nop
+; CHECK-NEXT:    ret
+; CHECK:       .section __patchable_function_entries,"awo",@progbits,f1{{$}}
+; 32:          .p2align 2
+; 32-NEXT:     .word .Lfunc_begin1
+; 64:          .p2align 3
+; 64-NEXT:     .quad .Lfunc_begin1
+  ret void
+}
+
+$f5 = comdat any
+define void @f5() "patchable-function-entry"="5" comdat {
+; CHECK-LABEL: f5:
+; CHECK-NEXT:  .Lfunc_begin2:
+; CHECK-COUNT-5: nop
+; CHECK-NEXT:    ret
+; CHECK:       .section __patchable_function_entries,"aGwo",@progbits,f5,comdat,f5{{$}}
+; 32:          .p2align 2
+; 32-NEXT:     .word .Lfunc_begin2
+; 64:          .p2align 3
+; 64-NEXT:     .quad .Lfunc_begin2
+  ret void
+}
+
+;; -fpatchable-function-entry=3,2
+;; "patchable-function-prefix" emits data before the function entry label.
+;; We emit 1-byte NOPs before the function entry, so that with a partial patch,
+;; the remaining instructions do not need to be modified.
+define void @f3_2() "patchable-function-entry"="1" "patchable-function-prefix"="2" {
+; CHECK-LABEL: .type f3_2,@function
+; CHECK-NEXT:  .Ltmp0: # @f3_2
+; CHECK-NEXT:    nop
+; CHECK-NEXT:    nop
+; CHECK-NEXT:  f3_2:
+; CHECK:       # %bb.0:
+; CHECK-NEXT:    nop
+; CHECK-NEXT:    addi sp, sp, -16
+;; .size does not include the prefix.
+; CHECK:      .Lfunc_end3:
+; CHECK-NEXT: .size f3_2, .Lfunc_end3-f3_2
+; CHECK:      .section __patchable_function_entries,"awo",@progbits,f3_2{{$}}
+; 32:         .p2align 2
+; 32-NEXT:    .word .Ltmp0
+; 64:         .p2align 3
+; 64-NEXT:    .quad .Ltmp0
+  %frame = alloca i8, i32 16
+  ret void
+}