This is an archive of the discontinued LLVM Phabricator instance.

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

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

Details

Summary

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

Repository
rL LLVM

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?

include/llvm/CodeGen/SelectionDAG.h
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);
MI.getOperand(3).setMetadata(DIExpr);
continue;

}

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!

lib/CodeGen/SelectionDAG/InstrEmitter.cpp
754 ↗(On Diff #141990)

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

lib/CodeGen/SelectionDAG/InstrEmitter.h
116 ↗(On Diff #141990)

don't repeat the name in the comment

lib/CodeGen/SelectionDAG/SDNodeDbgValue.h
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)

///

lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp
918 ↗(On Diff #141990)

perhaps use a range-based for?

931 ↗(On Diff #141990)

Do you have test coverage for both paths?

lib/CodeGen/SelectionDAG/SelectionDAG.cpp
7779 ↗(On Diff #141990)

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

http://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments

test/DebugInfo/Generic/debug-label-mi.ll
5 ↗(On Diff #141990)

can you also check the !dbg attachment?

aprantl added inline comments.Apr 11 2018, 10:00 AM
test/DebugInfo/Generic/debug-label-mi.ll
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.
lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp
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.
lib/CodeGen/AsmPrinter/AsmPrinter.cpp
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
lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp
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.