[WIP][CodeGen][DebugInfo][RISCV] Support scalable offsets in CFI #170607
Conversation
Currently, targets with scalable vectors (AArch64, RISCV) handle scalable offsets in cfi using cfi_escape expression. The problem is that CFIInstrInserter doesn't understand these expressions. We define new "pseudo" cfi instructions to support scalable offsets in CFI: (1) llvm_def_cfa_reg_scalable_offset - creates the new rule for calculating cfa: `deref(Reg + ScalableOffset * x + FixedOffset)` where `x` is the "scale" runtime constant. (2) llvm_reg_at_scalable_offset_from_cfa - creates the new rule to calculate previoius value of register `deref(cfa + ScalableOffset * x + FixedOffset)` where `x` is the "scale" runtime constant. (3) llvm_reg_at_scalable_offset_from_reg - creates the new rule to calculate previous value of register `deref(Reg2 + ScalableOffset * x + FixedOffset)`. This rule is to be used when offset from cfa is not known, but intead fixed offset from `Reg2` is known. All of these cfi_instructions will be "lowered" to escape expressions during final assembly emission. But during `CFIInstrInserter` these are not lowered yet, so their semantics can be understood without decoding cfi expressions. Since (1) and (2) depend on how target calculates scalable offsets, their lowering is target-dependent.
Currently we encode scalable offsets in CFIs using cfi_escape. But CFIInstrInserter can't handle cfi_escape.
Add support for scalable offsets.
llvmbot
added
backend:RISC-V
llvm:codegen
debuginfo
llvm:mc
|
@llvm/pr-subscribers-llvm-mc @llvm/pr-subscribers-backend-risc-v Author: Petr Penzin (ppenzin) ChangesSupport scalable offsets in CFI. This has been split out from https://github.com/mgudim/llvm-project/tree/save_csr_in_ra3, and is PR 2 out of 5. Co-authored-by: Mikhail Gudim <mgudim@ventanamicro.com> Patch is 54.49 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/170607.diff 22 Files Affected:
diff --git a/llvm/include/llvm/MC/MCDwarf.h b/llvm/include/llvm/MC/MCDwarf.h
index 9944a9a92ab1f..8a46ac64cdc87 100644
--- a/llvm/include/llvm/MC/MCDwarf.h
+++ b/llvm/include/llvm/MC/MCDwarf.h
@@ -15,6 +15,7 @@
#define LLVM_MC_MCDWARF_H
#include "llvm/ADT/MapVector.h"
+#include "llvm/ADT/SmallString.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/StringMap.h"
#include "llvm/ADT/StringRef.h"
@@ -514,6 +515,9 @@ class MCCFIInstruction {
OpRestoreState,
OpOffset,
OpLLVMDefAspaceCfa,
+ OpLLVMDefCfaRegScalableOffset,
+ OpLLVMRegAtScalableOffsetFromCfa,
+ OpLLVMRegAtScalableOffsetFromReg,
OpDefCfaRegister,
OpDefCfaOffset,
OpDefCfa,
@@ -547,6 +551,17 @@ class MCCFIInstruction {
unsigned Register;
unsigned Register2;
} RR;
+ struct {
+ unsigned Register;
+ int64_t ScalableOffset;
+ int64_t FixedOffset;
+ } ROO;
+ struct {
+ unsigned Register;
+ unsigned Register2;
+ int64_t ScalableOffset;
+ int64_t FixedOffset;
+ } RROO;
MCSymbol *CfiLabel;
} U;
OpType Operation;
@@ -579,6 +594,22 @@ class MCCFIInstruction {
U.CfiLabel = CfiLabel;
}
+ MCCFIInstruction(OpType Op, MCSymbol *L, unsigned R, int64_t ScalableOffset,
+ int64_t FixedOffset, SMLoc Loc, StringRef Comment = "")
+ : Label(L), Operation(Op), Loc(Loc), Comment(Comment) {
+ assert(Operation == OpLLVMDefCfaRegScalableOffset ||
+ Operation == OpLLVMRegAtScalableOffsetFromCfa);
+ U.ROO = {R, ScalableOffset, FixedOffset};
+ }
+
+ MCCFIInstruction(OpType Op, MCSymbol *L, unsigned R, unsigned R2,
+ int64_t ScalableOffset, int64_t FixedOffset, SMLoc Loc,
+ StringRef Comment = "")
+ : Label(L), Operation(Op), Loc(Loc), Comment(Comment) {
+ assert(Op == OpLLVMRegAtScalableOffsetFromReg);
+ U.RROO = {R, R2, ScalableOffset, FixedOffset};
+ }
+
public:
/// .cfi_def_cfa defines a rule for computing CFA as: take address from
/// Register and add Offset to it.
@@ -644,6 +675,41 @@ class MCCFIInstruction {
return MCCFIInstruction(OpRegister, L, Register1, Register2, Loc);
}
+ /// Create the new rule for calculating cfa: deref(Reg + ScalableOffset * x +
+ /// FixedOffset) where x is the runtime constant. This is a "pseudo" CFI
+ /// instruction - each target has to lower it into standard cfi directives.
+ static MCCFIInstruction
+ createLLVMDefCfaRegScalableOffset(MCSymbol *L, unsigned Reg,
+ int64_t ScalableOffset, int64_t FixedOffset,
+ SMLoc Loc = {}, StringRef Comment = "") {
+ return MCCFIInstruction(OpLLVMDefCfaRegScalableOffset, L, Reg,
+ ScalableOffset, FixedOffset, Loc, Comment);
+ }
+
+ /// Create the new rule for calculating the previous value (before the call)
+ /// of callee-saved register Reg: deref(cfa + ScalableOffset * x +
+ /// FixedOffset) where x is the runtime constant. This is a "pseudo" CFI
+ /// instruction - each target has to lower it into standard cfi directives.
+ static MCCFIInstruction createLLVMRegAtScalableOffsetFromCfa(
+ MCSymbol *L, unsigned Reg, int64_t ScalableOffset, int64_t FixedOffset,
+ SMLoc Loc = {}, StringRef Comment = "") {
+ return MCCFIInstruction(OpLLVMRegAtScalableOffsetFromCfa, L, Reg,
+ ScalableOffset, FixedOffset, Loc, Comment);
+ }
+
+ static MCCFIInstruction createLLVMRegAtScalableOffsetFromReg(
+ MCSymbol *L, unsigned Reg, unsigned Reg2, int64_t ScalableOffset,
+ int64_t FixedOffset, SMLoc Loc = {}, StringRef Comment = "") {
+ return MCCFIInstruction(OpLLVMRegAtScalableOffsetFromReg, L, Reg, Reg2,
+ ScalableOffset, FixedOffset, Loc, Comment);
+ }
+
+ /// Create the expression (FrameRegister + Offset) and write it to CFAExpr
+ static void createRegOffsetExpression(unsigned Reg, unsigned FrameReg,
+ int64_t ScalableOffset,
+ int64_t FixedOffset,
+ SmallString<64> &CFAExpr);
+
/// .cfi_window_save SPARC register window is saved.
static MCCFIInstruction createWindowSave(MCSymbol *L, SMLoc Loc = {}) {
return MCCFIInstruction(OpWindowSave, L, 0, INT64_C(0), Loc);
@@ -725,6 +791,10 @@ class MCCFIInstruction {
return U.RR.Register;
if (Operation == OpLLVMDefAspaceCfa)
return U.RIA.Register;
+ if (Operation == OpLLVMRegAtScalableOffsetFromCfa)
+ return U.ROO.Register;
+ if (Operation == OpLLVMRegAtScalableOffsetFromReg)
+ return U.RROO.Register;
assert(Operation == OpDefCfa || Operation == OpOffset ||
Operation == OpRestore || Operation == OpUndefined ||
Operation == OpSameValue || Operation == OpDefCfaRegister ||
@@ -733,8 +803,12 @@ class MCCFIInstruction {
}
unsigned getRegister2() const {
- assert(Operation == OpRegister);
- return U.RR.Register2;
+ if (Operation == OpRegister)
+ return U.RR.Register2;
+ if (Operation == OpLLVMDefCfaRegScalableOffset)
+ return U.ROO.Register;
+ assert(Operation == OpLLVMRegAtScalableOffsetFromReg);
+ return U.RROO.Register2;
}
unsigned getAddressSpace() const {
@@ -752,6 +826,22 @@ class MCCFIInstruction {
return U.RI.Offset;
}
+ int64_t getScalableOffset() const {
+ if (Operation == OpLLVMRegAtScalableOffsetFromReg)
+ return U.RROO.ScalableOffset;
+ assert(Operation == OpLLVMDefCfaRegScalableOffset ||
+ Operation == OpLLVMRegAtScalableOffsetFromCfa);
+ return U.ROO.ScalableOffset;
+ }
+
+ int64_t getFixedOffset() const {
+ if (Operation == OpLLVMRegAtScalableOffsetFromReg)
+ return U.RROO.FixedOffset;
+ assert(Operation == OpLLVMDefCfaRegScalableOffset ||
+ Operation == OpLLVMRegAtScalableOffsetFromCfa);
+ return U.ROO.FixedOffset;
+ }
+
MCSymbol *getCfiLabel() const {
assert(Operation == OpLabel);
return U.CfiLabel;
diff --git a/llvm/include/llvm/MC/MCStreamer.h b/llvm/include/llvm/MC/MCStreamer.h
index 79c715e3820a6..3429c2988423c 100644
--- a/llvm/include/llvm/MC/MCStreamer.h
+++ b/llvm/include/llvm/MC/MCStreamer.h
@@ -101,6 +101,25 @@ class LLVM_ABI MCTargetStreamer {
MCStreamer &getStreamer() { return Streamer; }
MCContext &getContext();
+ virtual void emitCFILLVMDefCfaRegScalableOffset(unsigned Register,
+ int64_t ScalableOffset,
+ int64_t FixedOffset,
+ SMLoc Loc = {}) {
+ llvm_unreachable("Not implemented!");
+ }
+ virtual void emitCFILLVMRegAtScalableOffsetFromCfa(unsigned Register,
+ int64_t ScalableOffset,
+ int64_t FixedOffset,
+ SMLoc Loc = {}) {
+ llvm_unreachable("Not implemented!");
+ }
+ virtual void emitCFILLVMRegAtScalableOffsetFromReg(unsigned Register,
+ unsigned Register2,
+ int64_t ScalableOffset,
+ int64_t FixedOffset,
+ SMLoc Loc = {}) {
+ llvm_unreachable("Not implemented!");
+ }
// Allow a target to add behavior to the EmitLabel of MCStreamer.
virtual void emitLabel(MCSymbol *Symbol);
// Allow a target to add behavior to the emitAssignment of MCStreamer.
diff --git a/llvm/lib/CodeGen/AsmPrinter/AsmPrinterDwarf.cpp b/llvm/lib/CodeGen/AsmPrinter/AsmPrinterDwarf.cpp
index 2a146eb15f709..55cd47daffd84 100644
--- a/llvm/lib/CodeGen/AsmPrinter/AsmPrinterDwarf.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/AsmPrinterDwarf.cpp
@@ -223,6 +223,24 @@ void AsmPrinter::emitCFIInstruction(const MCCFIInstruction &Inst) const {
OutStreamer->emitCFILLVMDefAspaceCfa(Inst.getRegister(), Inst.getOffset(),
Inst.getAddressSpace(), Loc);
break;
+ case MCCFIInstruction::OpLLVMDefCfaRegScalableOffset:
+ OutStreamer->AddComment(Inst.getComment());
+ OutStreamer->getTargetStreamer()->emitCFILLVMDefCfaRegScalableOffset(
+ Inst.getRegister2(), Inst.getScalableOffset(), Inst.getFixedOffset(),
+ Loc);
+ break;
+ case MCCFIInstruction::OpLLVMRegAtScalableOffsetFromCfa:
+ OutStreamer->AddComment(Inst.getComment());
+ OutStreamer->getTargetStreamer()->emitCFILLVMRegAtScalableOffsetFromCfa(
+ Inst.getRegister(), Inst.getScalableOffset(), Inst.getFixedOffset(),
+ Loc);
+ break;
+ case MCCFIInstruction::OpLLVMRegAtScalableOffsetFromReg:
+ OutStreamer->AddComment(Inst.getComment());
+ OutStreamer->getTargetStreamer()->emitCFILLVMRegAtScalableOffsetFromReg(
+ Inst.getRegister(), Inst.getRegister2(), Inst.getScalableOffset(),
+ Inst.getFixedOffset(), Loc);
+ break;
case MCCFIInstruction::OpOffset:
OutStreamer->emitCFIOffset(Inst.getRegister(), Inst.getOffset(), Loc);
break;
diff --git a/llvm/lib/CodeGen/CFIInstrInserter.cpp b/llvm/lib/CodeGen/CFIInstrInserter.cpp
index 667928304df50..1caa9f040e092 100644
--- a/llvm/lib/CodeGen/CFIInstrInserter.cpp
+++ b/llvm/lib/CodeGen/CFIInstrInserter.cpp
@@ -70,26 +70,30 @@ class CFIInstrInserter : public MachineFunctionPass {
#define INVALID_OFFSET INT_MAX
/// contains the location where CSR register is saved.
struct CSRSavedLocation {
- enum Kind { INVALID, REGISTER, CFA_OFFSET };
+ enum Kind { INVALID, REGISTER, CFA_OFFSET, REG_OFFSET };
CSRSavedLocation() {
K = Kind::INVALID;
Reg = 0;
- Offset = 0;
+ Offset = StackOffset::get(0, 0);
}
Kind K;
// Dwarf register number
unsigned Reg;
- // CFA offset
- int64_t Offset;
+ // Offset from CFA or Reg
+ StackOffset Offset;
bool isValid() const { return K != Kind::INVALID; }
bool operator==(const CSRSavedLocation &RHS) const {
+ if (K != RHS.K)
+ return false;
switch (K) {
case Kind::INVALID:
- return !RHS.isValid();
+ return true;
case Kind::REGISTER:
return Reg == RHS.Reg;
case Kind::CFA_OFFSET:
return Offset == RHS.Offset;
+ case Kind::REG_OFFSET:
+ return (Reg == RHS.Reg) && (Offset == RHS.Offset);
}
llvm_unreachable("Unknown CSRSavedLocation Kind!");
}
@@ -102,7 +106,12 @@ class CFIInstrInserter : public MachineFunctionPass {
OS << "In Dwarf register: " << Reg;
break;
case Kind::CFA_OFFSET:
- OS << "At CFA offset: " << Offset;
+ OS << "At CFA offset: (fixed: " << Offset.getFixed()
+ << ", scalable: " << Offset.getScalable() << ")";
+ break;
+ case Kind::REG_OFFSET:
+ OS << "At offset " << Offset.getFixed() << " from Dwarf register "
+ << Reg;
break;
}
}
@@ -111,9 +120,9 @@ class CFIInstrInserter : public MachineFunctionPass {
struct MBBCFAInfo {
MachineBasicBlock *MBB;
/// Value of cfa offset valid at basic block entry.
- int64_t IncomingCFAOffset = -1;
+ StackOffset IncomingCFAOffset = StackOffset::getFixed(-1);
/// Value of cfa offset valid at basic block exit.
- int64_t OutgoingCFAOffset = -1;
+ StackOffset OutgoingCFAOffset = StackOffset::getFixed(-1);
/// Value of cfa register valid at basic block entry.
unsigned IncomingCFARegister = 0;
/// Value of cfa register valid at basic block exit.
@@ -152,7 +161,7 @@ class CFIInstrInserter : public MachineFunctionPass {
/// Return the cfa offset value that should be set at the beginning of a MBB
/// if needed. The negated value is needed when creating CFI instructions
/// that set absolute offset.
- int64_t getCorrectCFAOffset(MachineBasicBlock *MBB) {
+ StackOffset getCorrectCFAOffset(MachineBasicBlock *MBB) {
return MBBVector[MBB->getNumber()].IncomingCFAOffset;
}
@@ -189,8 +198,8 @@ void CFIInstrInserter::calculateCFAInfo(MachineFunction &MF) {
for (MachineBasicBlock &MBB : MF) {
MBBCFAInfo &MBBInfo = MBBVector[MBB.getNumber()];
MBBInfo.MBB = &MBB;
- MBBInfo.IncomingCFAOffset = InitialOffset;
- MBBInfo.OutgoingCFAOffset = InitialOffset;
+ MBBInfo.IncomingCFAOffset = StackOffset::getFixed(InitialOffset);
+ MBBInfo.OutgoingCFAOffset = StackOffset::getFixed(InitialOffset);
MBBInfo.IncomingCFARegister = DwarfInitialRegister;
MBBInfo.OutgoingCFARegister = DwarfInitialRegister;
MBBInfo.IncomingCSRLocations.resize(NumRegs);
@@ -215,7 +224,7 @@ void CFIInstrInserter::calculateCFAInfo(MachineFunction &MF) {
void CFIInstrInserter::calculateOutgoingCFAInfo(MBBCFAInfo &MBBInfo) {
// Outgoing cfa offset set by the block.
- int64_t &OutgoingCFAOffset = MBBInfo.OutgoingCFAOffset;
+ StackOffset &OutgoingCFAOffset = MBBInfo.OutgoingCFAOffset;
OutgoingCFAOffset = MBBInfo.IncomingCFAOffset;
// Outgoing cfa register set by the block.
unsigned &OutgoingCFARegister = MBBInfo.OutgoingCFARegister;
@@ -243,22 +252,28 @@ void CFIInstrInserter::calculateOutgoingCFAInfo(MBBCFAInfo &MBBInfo) {
break;
}
case MCCFIInstruction::OpDefCfaOffset: {
- OutgoingCFAOffset = CFI.getOffset();
+ OutgoingCFAOffset = StackOffset::getFixed(CFI.getOffset());
break;
}
case MCCFIInstruction::OpAdjustCfaOffset: {
- OutgoingCFAOffset += CFI.getOffset();
+ OutgoingCFAOffset += StackOffset::getFixed(CFI.getOffset());
break;
}
case MCCFIInstruction::OpDefCfa: {
OutgoingCFARegister = CFI.getRegister();
- OutgoingCFAOffset = CFI.getOffset();
+ OutgoingCFAOffset = StackOffset::getFixed(CFI.getOffset());
+ break;
+ }
+ case MCCFIInstruction::OpLLVMDefCfaRegScalableOffset: {
+ OutgoingCFARegister = CFI.getRegister2();
+ OutgoingCFAOffset =
+ StackOffset::get(CFI.getFixedOffset(), CFI.getScalableOffset());
break;
}
case MCCFIInstruction::OpOffset: {
CSRSavedLocation &CSRLocation = OutgoingCSRLocations[CFI.getRegister()];
CSRLocation.K = CSRSavedLocation::Kind::CFA_OFFSET;
- CSRLocation.Offset = CFI.getOffset();
+ CSRLocation.Offset = StackOffset::getFixed(CFI.getOffset());
break;
}
case MCCFIInstruction::OpRegister: {
@@ -270,7 +285,23 @@ void CFIInstrInserter::calculateOutgoingCFAInfo(MBBCFAInfo &MBBInfo) {
case MCCFIInstruction::OpRelOffset: {
CSRSavedLocation &CSRLocation = OutgoingCSRLocations[CFI.getRegister()];
CSRLocation.K = CSRSavedLocation::Kind::CFA_OFFSET;
- CSRLocation.Offset = CFI.getOffset() - OutgoingCFAOffset;
+ CSRLocation.Offset =
+ StackOffset::getFixed(CFI.getOffset()) - OutgoingCFAOffset;
+ break;
+ }
+ case MCCFIInstruction::OpLLVMRegAtScalableOffsetFromCfa: {
+ CSRSavedLocation &CSRLocation = OutgoingCSRLocations[CFI.getRegister()];
+ CSRLocation.K = CSRSavedLocation::Kind::CFA_OFFSET;
+ CSRLocation.Offset =
+ StackOffset::get(CFI.getFixedOffset(), CFI.getScalableOffset());
+ break;
+ }
+ case MCCFIInstruction::OpLLVMRegAtScalableOffsetFromReg: {
+ CSRSavedLocation &CSRLocation = OutgoingCSRLocations[CFI.getRegister()];
+ CSRLocation.K = CSRSavedLocation::Kind::REG_OFFSET;
+ CSRLocation.Reg = CFI.getRegister2();
+ CSRLocation.Offset =
+ StackOffset::get(CFI.getFixedOffset(), CFI.getScalableOffset());
break;
}
case MCCFIInstruction::OpRestore: {
@@ -380,10 +411,19 @@ bool CFIInstrInserter::insertCFIInstrs(MachineFunction &MF) {
ForceFullCFA) {
// If both outgoing offset and register of a previous block don't match
// incoming offset and register of this block, or if this block begins a
- // section, add a def_cfa instruction with the correct offset and
- // register for this block.
- unsigned CFIIndex = MF.addFrameInst(MCCFIInstruction::cfiDefCfa(
- nullptr, MBBInfo.IncomingCFARegister, getCorrectCFAOffset(&MBB)));
+ // section, add a def_cfa (or llvm_def_cfa_reg_scalable_offset)
+ // instruction with the correct offset and register for this block.
+ StackOffset CorrectOffset = getCorrectCFAOffset(&MBB);
+ unsigned CFIIndex = (unsigned)(-1);
+ if (CorrectOffset.getScalable() == 0) {
+ CFIIndex = MF.addFrameInst(MCCFIInstruction::cfiDefCfa(
+ nullptr, MBBInfo.IncomingCFARegister, CorrectOffset.getFixed()));
+ } else {
+ CFIIndex =
+ MF.addFrameInst(MCCFIInstruction::createLLVMDefCfaRegScalableOffset(
+ nullptr, MBBInfo.IncomingCFARegister,
+ CorrectOffset.getScalable(), CorrectOffset.getFixed()));
+ }
BuildMI(*MBBInfo.MBB, MBBI, DL, TII->get(TargetOpcode::CFI_INSTRUCTION))
.addCFIIndex(CFIIndex);
InsertedCFIInstr = true;
@@ -391,8 +431,18 @@ bool CFIInstrInserter::insertCFIInstrs(MachineFunction &MF) {
// If outgoing offset of a previous block doesn't match incoming offset
// of this block, add a def_cfa_offset instruction with the correct
// offset for this block.
- unsigned CFIIndex = MF.addFrameInst(MCCFIInstruction::cfiDefCfaOffset(
- nullptr, getCorrectCFAOffset(&MBB)));
+ // If offset is scalable, add cfi_llvm_def_cfa_reg_scalable_offset
+ StackOffset CorrectOffset = getCorrectCFAOffset(&MBB);
+ unsigned CFIIndex = (unsigned)(-1);
+ if (CorrectOffset.getScalable() == 0) {
+ CFIIndex = MF.addFrameInst(MCCFIInstruction::cfiDefCfaOffset(
+ nullptr, CorrectOffset.getFixed()));
+ } else {
+ CFIIndex =
+ MF.addFrameInst(MCCFIInstruction::createLLVMDefCfaRegScalableOffset(
+ nullptr, MBBInfo.IncomingCFARegister,
+ CorrectOffset.getScalable(), CorrectOffset.getFixed()));
+ }
BuildMI(*MBBInfo.MBB, MBBI, DL, TII->get(TargetOpcode::CFI_INSTRUCTION))
.addCFIIndex(CFIIndex);
InsertedCFIInstr = true;
@@ -425,12 +475,21 @@ bool CFIInstrInserter::insertCFIInstrs(MachineFunction &MF) {
continue;
unsigned CFIIndex = (unsigned)(-1);
- if (HasToBeCSRLoc.K == CSRSavedLocation::Kind::CFA_OFFSET &&
- HasToBeCSRLoc.Offset != PrevOutgoingCSRLoc.Offset) {
- CFIIndex = MF.addFrameInst(
- MCCFIInstruction::createOffset(nullptr, i, HasToBeCSRLoc.Offset));
- } else if (HasToBeCSRLoc.K == CSRSavedLocation::Kind::REGISTER &&
- (HasToBeCSRLoc.Reg != PrevOutgoingCSRLoc.Reg)) {
+ switch (HasToBeCSRLoc.K) {
+ case CSRSavedLocation::Kind::CFA_OFFSET: {
+ StackOffset CorrectOffset = HasToBeCSRLoc.Offset;
+ if (CorrectOffset.getScalable() == 0) {
+ CFIIndex = MF.addFrameInst(MCCFIInstruction::createOffset(
+ nullptr, i, CorrectOffset.getFixed()));
+ } else {
+ CFIIndex = MF.addFrameInst(
+ MCCFIInstruction::createLLVMRegAtScalableOffsetFromCfa(
+ nullptr, i, CorrectOffset.getScalable(),
+ CorrectOffset.getFixed()));
+ }
+ break;
+ }
+ case CSRSavedLocation::Kind::REGISTER: {
unsigned NewReg = HasToBeCSRLoc.Reg;
unsigned DwarfEHReg = i;
if (NewReg == DwarfEHReg) {
@@ -440,8 +499,20 @@ bool CFIInstrInserter::insertCFIInstrs(MachineFunction &MF) {
CFIIndex = MF.addFrameInst(
MCCFIInstruction::createRegister(nullptr, i, HasToBeCSRLoc.Reg));
}
- } else
+ break;
+ }
+ case CSRSavedLocation::Kind::REG_OFFSET: {
+ StackOffset CorrectOffset = HasToBeCSRLoc.Offset;
+ unsigned CorrectReg = HasToBeCSRLoc.Reg;
+ CFIIndex = MF.addFrameInst(
+ MCCFIInstruction::createLLVMRegAtScalableOffsetFromReg(
+ nullptr, i, CorrectReg, CorrectOffset.getScalable(),
+ CorrectOffset.getFixed()));
+ break;
+ }
+ default:
llvm_unreachable("Unexpected CSR location.");
+ }
Bui...
[truncated]
|
🐧 Linux x64 Test Results
All tests passed but another part of the build failed. Click on a failure below to see the details. lib/DWARFCFIChecker/CMakeFiles/LLVMDWARFCFIChecker.dir/DWARFCFIState.cpp.otools/bolt/lib/Core/CMakeFiles/LLVMBOLTCore.dir/BinaryFunction.cpp.oIf these failures are unrelated to your changes (for example tests are broken or flaky at HEAD), please open an issue at https://github.com/llvm/llvm-project/issues and add the |
ppenzin
changed the title
|
This change has clean CI (the failures are coming from the one it is stacked on). I think I can make it stand on its own, but it might require a bit more careful DWARF verification. |
|
A macro question I have on this PR is why do we need this for the shrink wrapping stuff? Is this specific to shrink wrapping vector CSRs? If so, can we restrict the initial patch to only support scalar registers and delay this patch until later? I vaguely remember hearing something about a change to stack layout, is this connected to that? In another thread of query, we presumably support CFI for vector CSRs today. I haven't traced through that logic, but shouldn't whatever we do for scalable offsets there work for this use case? |
There was a problem hiding this comment.
I went looking for how we handle scalable offsets in the current vector frame setup and CSR handling, and found appendScalableVectorExpression and createDefCFAExpression in RISCVFrameLowering.cpp. Looking at the diff, I just noticed that there's an NFC buried in here -- we're replacing that logic with the new CFA Op being defined.
(Edit: TRI getOffsetOpcodes also seems relevant, and there might be some code duplication we can cleanup in the existing code.)
My overall question is Why? Why is the existing scheme not adequate for the new purpose?
I should caveat that I have minimal background with CFA/CFI/Dwarf, so there may be an "obvious" answer here. It's just not one I know.
This is why we need this: With my shrink-wrapping change there is NO PARTICULAR place for prolog / epilog. This means that: places where CFIs need to be emited now are calculated (I can point you to a patch later). Also since now CFIs can be anywhere we need to improve CFIInserter. If we don't fix / extend CFIInserter parest is miscompiled for example. In particular Another problem is that after CFI's are emited the machine instructions to which CFIs are "attached" can be moved around by post-ra scheduler. (Before prolog / epilog were a scheduling boundaries I think). I propose to to BUNDLE CFI instruction and corresponding machine instruction during PEI when these CFIs are created. I tried to do that and ran into some problems that BUNDLE is not supported in several passes. All of these seem pretty easy to fix though. It looks like people ran into the problem of BUNDLES not properly supported by all passes before (https://groups.google.com/g/llvm-dev/c/cRfXuc90Cr8?pli=1), so this could be another general improvement. Another option would be just to "glue" CFI and machine instruction in post ra scheduler, but there is no guarantee that some other pass will not move things around. Please let me know what you think about this approach. |
|
@ppenzin I am in the process of posting the commits myself, this may not be entirely ready. Please close these PRs. |
I would encourage an alternate approach. When you have a PR ready that you think directly replaces one of these, please link it directly from the individual PR and we can move discussion across. I'm entirely sympathetic to your wanting these commits credited to you, but I've been finding these much easier to understand than the existing patches. I really don't want to loose that until your patches are in a state where they really do explain what is being proposed. |
I should clarify the situation with branches / patches: (1) There exist 2 POC branches: (2) I have another branch: https://github.com/mgudim/llvm-project/tree/save_csr_in_ra3 where I split up all the work from (1) into small commits. This branch is still in the broken state and it is missing 3 - 4 commits. (3) As commits are ready in (2) and can be merged into This PR is just a commit taken from (2). I didn't post it myself because it's not ready yet, and I still have above 5 PRs to merge.
Sure we can keep it just to save this discussion, but in terms of content this is just a commit from (2). Actually, it would be better to move the discussion to #90819 I am going to post this explanation on the original PR too. |
Support scalable offsets in CFI.
This has been split out from @mgudim's save_csr_in_ra3 (from #90819), and is PR 2 out of 5.
determineCalleeSaves#170609Co-authored-by: Mikhail Gudim mgudim@ventanamicro.com