Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

Differential D35844 Diff 108098 lib/CodeGen/CFIInstrInserter.cpp

Changeset View

Standalone View

View Options

lib/CodeGen/CFIInstrInserter.cpp

  • This file was added.
//===------ CFIInstrInserter.cpp - Insert additional CFI instructions -----===//
//
// The LLVM Compiler Infrastructure
//
// This file is distributed under the University of Illinois Open Source
// License. See LICENSE.TXT for details.
//
//===----------------------------------------------------------------------===//
//
// Insert CFI instructions at the beginnings of basic blocks if needed. CFI
// instructions are inserted if basic blocks have incorrect offset or register
// set by prevoius blocks.
//
//===----------------------------------------------------------------------===//
#include "llvm/CodeGen/MachineFunctionPass.h"
#include "llvm/CodeGen/MachineInstrBuilder.h"
#include "llvm/CodeGen/MachineModuleInfo.h"
#include "llvm/CodeGen/Passes.h"
#include "llvm/Target/TargetFrameLowering.h"
MatzeBUnsubmitted
Done
ReplyInline Actions

Use doxygen comments:

/// \file This pass verifies ...
MatzeB: Use doxygen comments: ``` /// \file This pass verifies ... ```
#include "llvm/Target/TargetInstrInfo.h"
#include "llvm/Target/TargetMachine.h"
#include "llvm/Target/TargetSubtargetInfo.h"
using namespace llvm;
namespace {
class CFIInstrInserter : public MachineFunctionPass {
public:
CFIInstrInserter() : MachineFunctionPass(ID) {
initializeCFIInstrInserterPass(*PassRegistry::getPassRegistry());
}
bool runOnMachineFunction(MachineFunction &MF) override;
static char ID;
private:
thegamegUnsubmitted
Done
ReplyInline Actions

I think most of the members can be private here.

thegameg: I think most of the members can be private here.
StringRef getPassName() const override { return "CFI Instruction Inserter"; }
MatzeBUnsubmitted
Done
ReplyInline Actions

This feels slightly out of place as a member variable. Why not make it a return value of verify()?

MatzeB: This feels slightly out of place as a member variable. Why not make it a return value of…
// Check if incoming CFI information of a basic block matches outgoing CFI
// information of the previous block. If it doesn't, insert CFI instruction at
// the beginning of the block that corrects the CFA calculation rule for that
// block.
void CorrectCFA(MachineFunction &MF);
// Return the cfa offset value that should be set at the beginning of MBB if
// needed. The negated value is needed when creating CFI instructions that set
// absolute offset.
int getCorrectCFAOffset(MachineBasicBlock &MBB) {
return -MBB.getIncomingCFAOffset();
}
// Were any CFI instructions inserted
bool InsertedCFIInstr = false;
};
}
thegamegUnsubmitted
Not Done
ReplyInline Actions

Should this be guarded by the EXPENSIVE_CHECKS macro?

thegameg: Should this be guarded by the `EXPENSIVE_CHECKS` macro?
MatzeBUnsubmitted
Done
ReplyInline Actions

I wouldn't go as far as EXPENSIVE_CHECKS. But I think #ifndef NDEBUG would be good to skip the verification in release builds.

MatzeB: I wouldn't go as far as EXPENSIVE_CHECKS. But I think `#ifndef NDEBUG` would be good to skip…
char CFIInstrInserter::ID = 0;
INITIALIZE_PASS(CFIInstrInserter, "cfiinstrinserter",
"Check CFI info and insert CFI instructions if needed", false,
false)
FunctionPass *llvm::createCFIInstrInserter() { return new CFIInstrInserter(); }
bool CFIInstrInserter::runOnMachineFunction(MachineFunction &MF) {
bool NeedsDwarfCFI = (MF.getMMI().hasDebugInfo() ||
thegamegUnsubmitted
Done
ReplyInline Actions

In LLVM we usually use

struct MBBCFAInfo {
  [...]
};
thegameg: In LLVM we usually use struct MBBCFAInfo { [...] };
MF.getFunction()->needsUnwindTableEntry()) &&
(!MF.getTarget().getTargetTriple().isOSDarwin() &&
aprantlUnsubmitted
Done
ReplyInline Actions

please use /// for all comments describing declarations.

aprantl: please use `///` for all comments describing declarations.
!MF.getTarget().getTargetTriple().isOSWindows());
if (!NeedsDwarfCFI ||
!MF.getSubtarget().getFrameLowering()->maintainsCFIInfo())
return false;
// Insert appropriate CFI instructions for each MBB if CFA calculation rule
// needs to be corrected for that MBB.
CorrectCFA(MF);
return InsertedCFIInstr;
}
void CFIInstrInserter::CorrectCFA(MachineFunction &MF) {
MatzeBUnsubmitted
Done
ReplyInline Actions

MachineBasicBlocks have a dense numbering, so you can use a more efficient std::vector of length MachineFunction::getNumBlockIDs() instead. (See MachineTraceMetrics::BlockInfo for an example)

MatzeB: MachineBasicBlocks have a dense numbering, so you can use a more efficient std::vector of…
MachineBasicBlock &FirstMBB = MF.front();
MachineBasicBlock *PrevMBB = &FirstMBB;
const TargetInstrInfo *TII = MF.getSubtarget().getInstrInfo();
InsertedCFIInstr = false;
for (auto &MBB : MF) {
// Skip the first MBB in a function
if (MBB.getNumber() == FirstMBB.getNumber()) continue;
thegamegUnsubmitted
Done
ReplyInline Actions

Why not a reference instead of a pointer? It can never be null, right?

thegameg: Why not a reference instead of a pointer? It can never be null, right?
auto MBBI = MBB.begin();
DebugLoc DL = MBB.findDebugLoc(MBBI);
if (PrevMBB->getOutgoingCFAOffset() != MBB.getIncomingCFAOffset()) {
// If both outgoing offset and register of a previous block don't match
// incoming offset and register of this block, add a def_cfa instruction
// with the correct offset and register for this block.
if (PrevMBB->getOutgoingCFARegister() != MBB.getIncomingCFARegister()) {
unsigned CFIIndex = MF.addFrameInst(MCCFIInstruction::createDefCfa(
nullptr, MBB.getIncomingCFARegister(), getCorrectCFAOffset(MBB)));
BuildMI(MBB, MBBI, DL, TII->get(TargetOpcode::CFI_INSTRUCTION))
.addCFIIndex(CFIIndex);
// 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.
} else {
unsigned CFIIndex =
MF.addFrameInst(MCCFIInstruction::createDefCfaOffset(
nullptr, getCorrectCFAOffset(MBB)));
BuildMI(MBB, MBBI, DL, TII->get(TargetOpcode::CFI_INSTRUCTION))
.addCFIIndex(CFIIndex);
}
InsertedCFIInstr = true;
// If outgoing register of a previous block doesn't match incoming
// register of this block, add a def_cfa_register instruction with the
// correct register for this block.
MatzeBUnsubmitted
Done
ReplyInline Actions

We tend to use lowercase pass names here. So maybe "cfi-instr-inserter"?

MatzeB: We tend to use lowercase pass names here. So maybe `"cfi-instr-inserter"`?
} else if (PrevMBB->getOutgoingCFARegister() !=
MBB.getIncomingCFARegister()) {
unsigned CFIIndex =
MF.addFrameInst(MCCFIInstruction::createDefCfaRegister(
nullptr, MBB.getIncomingCFARegister()));
BuildMI(MBB, MBBI, DL, TII->get(TargetOpcode::CFI_INSTRUCTION))
.addCFIIndex(CFIIndex);
InsertedCFIInstr = true;
}
PrevMBB = &MBB;
}
}
thegamegUnsubmitted
Not Done
ReplyInline Actions

No need for struct here.

thegameg: No need for `struct` here.
thegamegUnsubmitted
Done
ReplyInline Actions

You could limit the scope of SuccInfo to the loop body.

thegameg: You could limit the scope of `SuccInfo` to the loop body.
thegamegUnsubmitted
Done
ReplyInline Actions

I think you can assume the reference is always non-null here, right?

thegameg: I think you can assume the reference is always non-null here, right?
MatzeBUnsubmitted
Done
ReplyInline Actions

References must be non-null otherwise it's undefined behavior. This means the compiler will most likely optimize this assert away and it won't do anything even if the reference happened to illegally be nullptr.

MatzeB: References must be non-null otherwise it's undefined behavior. This means the compiler will…
thegamegUnsubmitted
Done
ReplyInline Actions

You can already initialize the variables here.

thegameg: You can already initialize the variables here.
MatzeBUnsubmitted
Done
ReplyInline Actions

Please avoid auto in cases where the actual type is only slightly longer. I think that is friendlier to readers of the code. (same for a number of other loops below).

MatzeB: Please avoid `auto` in cases where the actual type is only slightly longer. I think that is…
MatzeBUnsubmitted
Done
ReplyInline Actions

You could use for (MachineInstr &MI : make_range(MBBInfo->MBB->instr_begin(), MBBInfo->MBB->instr_end()) { ... } here.

MatzeB: You could use `for (MachineInstr &MI : make_range(MBBInfo->MBB->instr_begin(), MBBInfo->MBB…
MatzeBUnsubmitted
Done
ReplyInline Actions

You could move this declaration down to the use (and combine it with the first assignment).

MatzeB: You could move this declaration down to the use (and combine it with the first assignment).
MatzeBUnsubmitted
Done
ReplyInline Actions

The declarations could be move downwards to their first use.

MatzeB: The declarations could be move downwards to their first use.
thegamegUnsubmitted
Done
ReplyInline Actions

This could be a const MBBCFAInfo * to avoid copies.

thegameg: This could be a `const MBBCFAInfo *` to avoid copies.
thegamegUnsubmitted
Done
ReplyInline Actions

And this const MBBCFAInfo &.

thegameg: And this `const MBBCFAInfo &`.
thegamegUnsubmitted
Done
ReplyInline Actions

Same.

thegameg: Same.
thegamegUnsubmitted
Done
ReplyInline Actions

Same.

thegameg: Same.
thegamegUnsubmitted
Done
ReplyInline Actions

Same.

thegameg: Same.