[RISCV] Add an option to enable CFIInstrInserter. #164477
Conversation
|
@llvm/pr-subscribers-backend-risc-v Author: Mikhail Gudim (mgudim) ChangesFull diff: https://github.com/llvm/llvm-project/pull/164477.diff 3 Files Affected:
diff --git a/llvm/lib/Target/RISCV/RISCVFrameLowering.cpp b/llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
index b37b7405a660f..19524d016c06f 100644
--- a/llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
+++ b/llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
@@ -2507,3 +2507,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 6af63a4885f35..87980dfb09f96 100644
--- a/llvm/lib/Target/RISCV/RISCVFrameLowering.h
+++ b/llvm/lib/Target/RISCV/RISCVFrameLowering.h
@@ -23,6 +23,9 @@ class RISCVFrameLowering : public TargetFrameLowering {
public:
explicit RISCVFrameLowering(const RISCVSubtarget &STI);
+ int getInitialCFAOffset(const MachineFunction &MF) const override;
+ Register getInitialCFARegister(const MachineFunction &MF) const override;
+
void emitPrologue(MachineFunction &MF, MachineBasicBlock &MBB) const override;
void emitEpilogue(MachineFunction &MF, MachineBasicBlock &MBB) const override;
diff --git a/llvm/lib/Target/RISCV/RISCVTargetMachine.cpp b/llvm/lib/Target/RISCV/RISCVTargetMachine.cpp
index f81b1e1260ee3..077dbcc7d9003 100644
--- a/llvm/lib/Target/RISCV/RISCVTargetMachine.cpp
+++ b/llvm/lib/Target/RISCV/RISCVTargetMachine.cpp
@@ -103,6 +103,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_ABI LLVM_EXTERNAL_VISIBILITY void LLVMInitializeRISCVTarget() {
RegisterTargetMachine<RISCVTargetMachine> X(getTheRISCV32Target());
RegisterTargetMachine<RISCVTargetMachine> Y(getTheRISCV64Target());
@@ -169,7 +174,7 @@ 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);
+ setCFIFixup(!EnableCFIInstrInserter);
}
const RISCVSubtarget *
@@ -576,6 +581,9 @@ void RISCVPassConfig::addPreEmitPass2() {
addPass(createUnpackMachineBundles([&](const MachineFunction &MF) {
return MF.getFunction().getParent()->getModuleFlag("kcfi");
}));
+
+ if (EnableCFIInstrInserter)
+ addPass(createCFIInstrInserter());
}
void RISCVPassConfig::addMachineSSAOptimization() {
|
mgudim
force-pushed
the
enable_cfi_option
branch
from
aad0c85 to
bdd6628
Compare
mgudim
changed the title
|
ping |
lenary
left a comment
lenary
left a comment
There was a problem hiding this comment.
Please add some tests showing what this ends up doing.
|
LGTM |
|
I'm not actually sure the value of that test. What I'm trying to get an answer to is "why is this pass being added?", but I also haven't had the time to go and review what CFIInserter does and why - I know it's (by LLVM's standards) a fairly recent addition. (Your test shows CFI Inserter is being added, but not what effect that has, just the debug output from it being added) If there are no (CodeGen) changes to existing tests, then I guess there's no problem and we can proceed, especially if it unblocks things we know are improvements. |
This is wrong. CFI Inserter is a lot older than I thought it was. I was confusing it with CFI Fixup, which is only 4 years old. |
lenary
left a comment
lenary
left a comment
There was a problem hiding this comment.
LGTM. Sorry for taking a while to get to that decision.
basically The first What CFIInserter does is it compares CFI from layout predecessor to CFG predecessors and inserters more cfis if needed. In this case the result would look like this: It turns out that current CFIInserter implementation does not handle some cases. I created test cases for those in separate MRs (#164477 (comment)) Also I have fixes for these . So these are useful even without shrink wrapping changes which I want to make. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/11/builds/28266 Here is the relevant piece of the build log for the reference |
|
@mgudim -- we are seeing the test failures on build bot Can you please fix or revert? |
@Prabhuk sorry about breaking the build. Here's a fix: #168525 . If you review it, I'll merge it. Or you can merge it too |
6 participants
No description provided.