Page MenuHomePhabricator

[DebugInfo] Convert intrinsic llvm.dbg.label to MachineInstr.

Authored by HsiangKai on Apr 5 2018, 5:10 PM.



In order to convert LLVM IR to MachineInstr, we need a new TargetOpcode,
DBG_LABEL, to 'lower' intrinsic llvm.dbg.label. The patch
creates this new TargetOpcode and convert intrinsic llvm.dbg.label to
MachineInstr through SelectionDAG.

In SelectionDAG, debug information is stored in SDDbgInfo. We create a
new data member of SDDbgInfo for labels and use the new data member,
SDDbgLabel, to create DBG_LABEL MachineInstr.

The new DBG_LABEL MachineInstr uses label metadata from LLVM IR as its
parameter. So, the backend could get metadata information of labels from
DBG_LABEL MachineInstr.

Diff Detail


Event Timeline

HsiangKai created this revision.Apr 5 2018, 5:10 PM

Thanks, this looks mostly fine! Have you considered just allowing the DBG_VALUE MachineInstr to carry a DILabel insetad of just DILocalVariables? Would that simplify the code or make it harder to understand?

152 ↗(On Diff #141241)

I suppose this depends on the language, but I'd expect most C++ functions to at most a handful of labels and usually zero. Perhaps use a SmallVector size of 1, 2, or 4, or even a std::vector?

HsiangKai updated this revision to Diff 141990.Apr 11 2018, 6:16 AM

Update according to review comments.

Thanks, this looks mostly fine! Have you considered just allowing the DBG_VALUE MachineInstr to carry a DILabel insetad of just DILocalVariables? Would that simplify the code or make it harder to understand?

The formats of DBG_VALUE are

DBG_VALUE Reg, 0, DILocalVariable, DIExpression
DBG_VALUE FrameIndex, Imm, DILocalVariable, DIExpression
DBG_VALUE Imm, Imm, DILocalVariable, DIExpression

When processing DBG_VALUE, there are assumptions about these operands. Take a snippet from lib/CodeGen/PrologEpilogInserter.cpp as an example,

if (MI.isDebugValue()) {

assert(i == 0 && "Frame indices can only appear as the first "
                 "operand of a DBG_VALUE machine instruction");
unsigned Reg;
int64_t Offset =
    TFI->getFrameIndexReference(Fn, MI.getOperand(0).getIndex(), Reg);
MI.getOperand(0).ChangeToRegister(Reg, false /*isDef*/);
auto *DIExpr = DIExpression::prepend(MI.getDebugExpression(),
                                     DIExpression::NoDeref, Offset);


If we add the possibility of operand(0) as DILabel, it will complicate the implementation.

I think to create DBG_LABEL is easier to understand and easier to maintain.

Okay, let's do a separate machine instruction then!

754 ↗(On Diff #141990)

Again, the comment should be on the declaration, not the definition.

116 ↗(On Diff #141990)

don't repeat the name in the comment

118 ↗(On Diff #141990)

Please don't repeat the class name in comments.

120 ↗(On Diff #141990)

extra newline

130 ↗(On Diff #141990)


133 ↗(On Diff #141990)


136 ↗(On Diff #141990)


918 ↗(On Diff #141990)

perhaps use a range-based for?

931 ↗(On Diff #141990)

Do you have test coverage for both paths?

7779 ↗(On Diff #141990)

The comment should be on the function *declaration*, and not repeat the function name:
/// Add a dbg_label SDNode.

5 ↗(On Diff #141990)

can you also check the !dbg attachment?

aprantl added inline comments.Apr 11 2018, 10:00 AM
27 ↗(On Diff #141990)

you can reduce the size of the testcase by manually making all !dbg attachments of the other instructions point to the same location. That will make it more obvious what the relevant part of the test is.

HsiangKai updated this revision to Diff 142531.Apr 14 2018, 6:59 PM

Update according to reviewers' comments.

HsiangKai updated this revision to Diff 142633.Apr 16 2018, 7:46 AM
  1. Use range-based for in lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp:918
  2. Check !dbg attachment in the test case.
931 ↗(On Diff #141990)

I will try to create test cases for both paths.

HsiangKai marked 11 inline comments as done.Apr 16 2018, 8:02 AM
aprantl accepted this revision.Apr 16 2018, 9:11 AM
aprantl added inline comments.
915 ↗(On Diff #142633)

/// This method handles the target-independent form
don't repeat the function name.

This revision is now accepted and ready to land.Apr 16 2018, 9:11 AM
HsiangKai marked 2 inline comments as done.
  • Update the test case.
HsiangKai marked 2 inline comments as done.

Add test case.

HsiangKai added inline comments.Apr 26 2018, 11:10 PM
931 ↗(On Diff #141990)

Add test case 'test/DebugInfo/Generic/debug-label-opt.ll' for both paths.

This revision was automatically updated to reflect the committed changes.