[CFIInstrInserter] Tests for scenarios not currently handled by `CFIInstrInserter`. #128211

mgudim · 2025-02-21T18:00:05Z

We add an option to enable CFIInstrInserter on RISCV (false by default). On RISCV there exists a realistic scenario (involing scalable vectors) where we need CFIInstrInserter to handle cfi_escape. We add a test for that. Also, we add a test for the scenario when a callee-saved register is saved at different locations.

llvmbot · 2025-02-21T18:00:43Z

@llvm/pr-subscribers-backend-risc-v

Author: Mikhail Gudim (mgudim)

Changes

We add an option to enable CFIInstrInserter on RISCV (false by default). This lets us add a test showing the need to handle cfi_escape in CFIInstrInserter.

Full diff: https://github.com/llvm/llvm-project/pull/128211.diff

4 Files Affected:

(modified) llvm/lib/Target/RISCV/RISCVFrameLowering.cpp (+9)
(modified) llvm/lib/Target/RISCV/RISCVFrameLowering.h (+2)
(modified) llvm/lib/Target/RISCV/RISCVTargetMachine.cpp (+12-1)
(added) llvm/test/CodeGen/RISCV/cfi-escape.mir (+88)

diff --git a/llvm/lib/Target/RISCV/RISCVFrameLowering.cpp b/llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
index 6abf45591d78e..96b0ef0ec40aa 100644
--- a/llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
+++ b/llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
@@ -2239,3 +2239,12 @@ void RISCVFrameLowering::inlineStackProbe(MachineFunction &MF,
     }
   }
 }
+
+int RISCVFrameLowering::getInitialCFAOffset(const MachineFunction &MF) const {
+  return 0;
+}
+
+Register
+RISCVFrameLowering::getInitialCFARegister(const MachineFunction &MF) const {
+  return RISCV::X2;
+}
diff --git a/llvm/lib/Target/RISCV/RISCVFrameLowering.h b/llvm/lib/Target/RISCV/RISCVFrameLowering.h
index d013755ce58a0..faad93a513258 100644
--- a/llvm/lib/Target/RISCV/RISCVFrameLowering.h
+++ b/llvm/lib/Target/RISCV/RISCVFrameLowering.h
@@ -112,6 +112,8 @@ class RISCVFrameLowering : public TargetFrameLowering {
                                    const DebugLoc &DL, int64_t Amount,
                                    MachineInstr::MIFlag Flag, bool EmitCFI,
                                    bool DynAllocation) const;
+  int getInitialCFAOffset(const MachineFunction &MF) const override;
+  Register getInitialCFARegister(const MachineFunction &MF) const override;
 };
 } // namespace llvm
 #endif
diff --git a/llvm/lib/Target/RISCV/RISCVTargetMachine.cpp b/llvm/lib/Target/RISCV/RISCVTargetMachine.cpp
index 167dbb53c5950..e3cfd089377e2 100644
--- a/llvm/lib/Target/RISCV/RISCVTargetMachine.cpp
+++ b/llvm/lib/Target/RISCV/RISCVTargetMachine.cpp
@@ -117,6 +117,11 @@ static cl::opt<bool>
                            cl::desc("Enable Machine Pipeliner for RISC-V"),
                            cl::init(false), cl::Hidden);
 
+static cl::opt<bool>
+    EnableCFIInstrInserter("riscv-enable-cfi-instr-inserter",
+                           cl::desc("Enable CFI Instruction Inserter for RISC-V"),
+                           cl::init(false), cl::Hidden);
+
 extern "C" LLVM_EXTERNAL_VISIBILITY void LLVMInitializeRISCVTarget() {
   RegisterTargetMachine<RISCVTargetMachine> X(getTheRISCV32Target());
   RegisterTargetMachine<RISCVTargetMachine> Y(getTheRISCV64Target());
@@ -187,7 +192,10 @@ RISCVTargetMachine::RISCVTargetMachine(const Target &T, const Triple &TT,
   if (TT.isOSFuchsia() && !TT.isArch64Bit())
     report_fatal_error("Fuchsia is only supported for 64-bit");
 
-  setCFIFixup(true);
+  if (EnableCFIInstrInserter)
+    setCFIFixup(false);
+  else
+    setCFIFixup(true);
 }
 
 const RISCVSubtarget *
@@ -584,6 +592,9 @@ void RISCVPassConfig::addPreEmitPass2() {
   addPass(createUnpackMachineBundles([&](const MachineFunction &MF) {
     return MF.getFunction().getParent()->getModuleFlag("kcfi");
   }));
+
+  if (EnableCFIInstrInserter)
+    addPass(createCFIInstrInserter());
 }
 
 void RISCVPassConfig::addMachineSSAOptimization() {
diff --git a/llvm/test/CodeGen/RISCV/cfi-escape.mir b/llvm/test/CodeGen/RISCV/cfi-escape.mir
new file mode 100644
index 0000000000000..a15825caa9fb8
--- /dev/null
+++ b/llvm/test/CodeGen/RISCV/cfi-escape.mir
@@ -0,0 +1,88 @@
+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 5
+# RUN: llc %s -mtriple=riscv64 -mattr=+v \
+# RUN: -run-pass=prologepilog,cfi-instr-inserter  \
+# RUN: -riscv-enable-cfi-instr-inserter=true \
+# RUN: | FileCheck %s
+#
+# In this test prolog will be inserted in bb.3. We need to save the scalable vector register v1.
+# This will emit cfi_escape in bb.3. We need to emit the same cfi_escape in the begining of bb.2.
+# Currently, CFIInstrInserter doesn't handle escape, so we have wrong cfi in this example.
+
+--- |
+
+  define riscv_vector_cc void @test0(ptr %p0, ptr %p1) #0 {
+    entry:
+    %v = load <4 x i32>, ptr %p0, align 16
+    store <4 x i32> %v, ptr %p1, align 16
+    ret void
+  }
+
+  attributes #0 = { "target-features"="+v" }
+
+...
+---
+name:            test0
+tracksRegLiveness: true
+frameInfo:
+  savePoint:       '%bb.3'
+  restorePoint:    '%bb.2'
+body:             |
+  ; CHECK-LABEL: name: test0
+  ; CHECK: bb.0.entry:
+  ; CHECK-NEXT:   successors: %bb.3(0x40000000), %bb.1(0x40000000)
+  ; CHECK-NEXT:   liveins: $x10, $x11, $v1
+  ; CHECK-NEXT: {{  $}}
+  ; CHECK-NEXT:   BEQ $x10, $x0, %bb.3
+  ; CHECK-NEXT: {{  $}}
+  ; CHECK-NEXT: bb.1:
+  ; CHECK-NEXT:   liveins: $v1
+  ; CHECK-NEXT: {{  $}}
+  ; CHECK-NEXT:   PseudoRET
+  ; CHECK-NEXT: {{  $}}
+  ; CHECK-NEXT: bb.2:
+  ; CHECK-NEXT:   successors: %bb.1(0x80000000)
+  ; CHECK-NEXT: {{  $}}
+  ; CHECK-NEXT:   CFI_INSTRUCTION def_cfa_offset 16
+  ; CHECK-NEXT:   $x10 = ADDI $x2, 16
+  ; CHECK-NEXT:   $v1 = frame-destroy VL1RE8_V killed $x10 :: (load unknown-size from %stack.0, align 8)
+  ; CHECK-NEXT:   $x10 = frame-destroy PseudoReadVLENB
+  ; CHECK-NEXT:   $x2 = frame-destroy ADD $x2, killed $x10
+  ; CHECK-NEXT:   frame-destroy CFI_INSTRUCTION def_cfa $x2, 16
+  ; CHECK-NEXT:   frame-destroy CFI_INSTRUCTION restore $v1
+  ; CHECK-NEXT:   $x2 = frame-destroy ADDI $x2, 16
+  ; CHECK-NEXT:   frame-destroy CFI_INSTRUCTION def_cfa_offset 0
+  ; CHECK-NEXT:   PseudoBR %bb.1
+  ; CHECK-NEXT: {{  $}}
+  ; CHECK-NEXT: bb.3:
+  ; CHECK-NEXT:   successors: %bb.2(0x80000000)
+  ; CHECK-NEXT:   liveins: $x10, $x11, $v1
+  ; CHECK-NEXT: {{  $}}
+  ; CHECK-NEXT:   $x2 = frame-setup ADDI $x2, -16
+  ; CHECK-NEXT:   frame-setup CFI_INSTRUCTION def_cfa_offset 16
+  ; CHECK-NEXT:   $x12 = frame-setup PseudoReadVLENB
+  ; CHECK-NEXT:   $x2 = frame-setup SUB $x2, killed $x12
+  ; CHECK-NEXT:   frame-setup CFI_INSTRUCTION escape 0x0f, 0x0d, 0x72, 0x00, 0x11, 0x10, 0x22, 0x11, 0x01, 0x92, 0xa2, 0x38, 0x00, 0x1e, 0x22
+  ; CHECK-NEXT:   $x12 = ADDI $x2, 16
+  ; CHECK-NEXT:   frame-setup VS1R_V killed $v1, killed $x12 :: (store unknown-size into %stack.0, align 8)
+  ; CHECK-NEXT:   frame-setup CFI_INSTRUCTION escape 0x10, 0x61, 0x08, 0x11, 0x7f, 0x92, 0xa2, 0x38, 0x00, 0x1e, 0x22
+  ; CHECK-NEXT:   dead $x0 = PseudoVSETIVLI 4, 208 /* e32, m1, ta, ma */, implicit-def $vl, implicit-def $vtype
+  ; CHECK-NEXT:   renamable $v1 = PseudoVLE32_V_M1 undef renamable $v1, killed renamable $x10, 4, 5 /* e32 */, 2 /* tu, ma */, implicit $vl, implicit $vtype
+  ; CHECK-NEXT:   PseudoVSE32_V_M1 killed renamable $v1, killed renamable $x11, 4, 5 /* e32 */, implicit $vl, implicit $vtype
+  ; CHECK-NEXT:   PseudoBR %bb.2
+  bb.0.entry:
+    liveins: $x10, $x11
+    BEQ $x10, $x0, %bb.3
+
+  bb.1:
+    PseudoRET
+
+  bb.2:
+    PseudoBR %bb.1
+
+  bb.3:
+    liveins: $x10, $x11
+    dead $x0 = PseudoVSETIVLI 4, 208, implicit-def $vl, implicit-def $vtype
+    renamable $v1 = PseudoVLE32_V_M1 undef renamable $v1, killed renamable $x10, 4, 5, 2, implicit $vl, implicit $vtype
+    PseudoVSE32_V_M1 killed renamable $v1, killed renamable $x11, 4, 5, implicit $vl, implicit $vtype
+    PseudoBR %bb.2
+...

github-actions · 2025-02-21T18:03:37Z

✅ With the latest revision this PR passed the C/C++ code formatter.

mgudim · 2025-02-21T18:12:12Z

The fix I propose for this is:
#125104

Eventually, this is needed for #90819

topperc · 2025-02-21T18:48:08Z

llvm/lib/Target/RISCV/RISCVTargetMachine.cpp

@@ -187,7 +192,10 @@ RISCVTargetMachine::RISCVTargetMachine(const Target &T, const Triple &TT,
  if (TT.isOSFuchsia() && !TT.isArch64Bit())
    report_fatal_error("Fuchsia is only supported for 64-bit");

-  setCFIFixup(true);
+  if (EnableCFIInstrInserter)


setCFIFixup(!EnableCFIInstrInserter)?

that's awesome, thanks!

topperc · 2025-02-21T18:48:58Z

llvm/test/CodeGen/RISCV/cfi-escape.mir

+# This will emit cfi_escape in bb.3. We need to emit the same cfi_escape in the begining of bb.2.
+# Currently, CFIInstrInserter doesn't handle escape, so we have wrong cfi in this example.
+
+--- |


Do we need the IR section?

yes, it specifies calling convention

`CFIInstrInserter`. We add an option to enable `CFIInstrInserter` on RISCV (`false` by default). On RISCV there exists a realistic scenario (involing scalable vectors) where we need CFIInstrInserter to handle `cfi_escape`. We add a test for that. Also, we add a test for the scenario when a callee-saved register is saved at different locations.

michaelmaitland · 2025-02-24T15:58:00Z

llvm/test/CodeGen/RISCV/cfi-escape.mir

+    dead $x0 = PseudoVSETIVLI 4, 208, implicit-def $vl, implicit-def $vtype
+    renamable $v1 = PseudoVLE32_V_M1 undef renamable $v1, killed renamable $x10, 4, 5, 2, implicit $vl, implicit $vtype
+    PseudoVSE32_V_M1 killed renamable $v1, killed renamable $x11, 4, 5, implicit $vl, implicit $vtype
+    PseudoBR %bb.2


Why do we branch to bb.2 which branches to bb.1 which returns? Can it be simplified?

The instructions by themselves are silly, but we only care about block layout. The block bb.2 is there because "We need to emit the same cfi_escape in the begining of bb.2"

or you mean I should merge bb1 into bb2?

Yes

mgudim requested review from michaelmaitland and topperc February 21, 2025 18:00

llvmbot added the backend:RISC-V label Feb 21, 2025

mgudim force-pushed the cfi_escape branch from a16a0f1 to e22d20f Compare February 21, 2025 18:14

topperc reviewed Feb 21, 2025

View reviewed changes

mgudim force-pushed the cfi_escape branch from e22d20f to f3768ab Compare February 24, 2025 13:15

mgudim force-pushed the cfi_escape branch from f3768ab to a925cc8 Compare February 24, 2025 13:22

michaelmaitland reviewed Feb 24, 2025

View reviewed changes

mgudim changed the title ~~[CFIInstrInserter] A test showing the need to handle cfi_escape.~~ [CFIInstrInserter] Tests for scenarios not currently handled by CFIInstrInserter. Feb 24, 2025

mgudim requested review from petar-jovanovic February 24, 2025 16:51

mgudim closed this May 2, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CFIInstrInserter] Tests for scenarios not currently handled by `CFIInstrInserter`. #128211

[CFIInstrInserter] Tests for scenarios not currently handled by `CFIInstrInserter`. #128211

mgudim commented Feb 21, 2025 •

edited

Loading

llvmbot commented Feb 21, 2025

github-actions bot commented Feb 21, 2025 •

edited

Loading

mgudim commented Feb 21, 2025

topperc Feb 21, 2025

mgudim Feb 24, 2025

topperc Feb 21, 2025

mgudim Feb 21, 2025

michaelmaitland Feb 24, 2025

mgudim Feb 24, 2025

michaelmaitland Feb 24, 2025

[CFIInstrInserter] Tests for scenarios not currently handled by CFIInstrInserter. #128211

[CFIInstrInserter] Tests for scenarios not currently handled by CFIInstrInserter. #128211

Conversation

mgudim commented Feb 21, 2025 • edited Loading

llvmbot commented Feb 21, 2025

github-actions bot commented Feb 21, 2025 • edited Loading

mgudim commented Feb 21, 2025

topperc Feb 21, 2025

Choose a reason for hiding this comment

mgudim Feb 24, 2025

Choose a reason for hiding this comment

topperc Feb 21, 2025

Choose a reason for hiding this comment

mgudim Feb 21, 2025

Choose a reason for hiding this comment

michaelmaitland Feb 24, 2025

Choose a reason for hiding this comment

mgudim Feb 24, 2025

Choose a reason for hiding this comment

michaelmaitland Feb 24, 2025

Choose a reason for hiding this comment

[CFIInstrInserter] Tests for scenarios not currently handled by `CFIInstrInserter`. #128211

[CFIInstrInserter] Tests for scenarios not currently handled by `CFIInstrInserter`. #128211

mgudim commented Feb 21, 2025 •

edited

Loading

github-actions bot commented Feb 21, 2025 •

edited

Loading