This is an archive of the discontinued LLVM Phabricator instance.

[Debug info] Teach the SDag type legalizer how to split up debug info for integer values that are split into a hi and lo part.
ClosedPublic

Authored by JDevlieghere on Aug 16 2017, 1:01 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

JDevlieghere created this revision.Aug 16 2017, 1:01 PM
aprantl edited edge metadata.Aug 16 2017, 1:20 PM

Thanks, I have a couple of smaller nitpicks, but this generally seems good to me.

llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.cpp
829 ↗(On Diff #111404)

This comment basically just repeats the name of the function. I would either document what the function does, i.e. generate fragment expressions for the split up value, or remove it entirely.

837 ↗(On Diff #111404)

llvm coding style prefers early exit over nesting, i.e:

if (Dbg->getKind() !=SDNode)
   break;
838 ↗(On Diff #111404)

perhaps change the return type in SDDbgValue instead?

842 ↗(On Diff #111404)

why is the &* necessary?

llvm/test/DebugInfo/MSP430/sdagsplit-1.ll
20 ↗(On Diff #111404)

Hard-coding the metadata node numbers is not a good idea. Since you aren't checking their contents, why check for them at all?
We *should* be checking for the new DIExpressions on the other hand.

41 ↗(On Diff #111404)

can you remove all non-essential attributes (at least everything in quotes)?

llvm/test/DebugInfo/X86/sdagsplit-1.ll
18 ↗(On Diff #111404)

Probably better to convert this to checking the MIR output, too.

41 ↗(On Diff #111404)

ditto

JDevlieghere marked 7 inline comments as done.

Thank you for the feedback @aprantl!

Fix typo in X86 test.

aprantl accepted this revision.Aug 17 2017, 9:24 AM

Thanks!

llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.cpp
853 ↗(On Diff #111436)

No need to do this now, but we might want to think about adding this functionality either to DIBuilder or IR/DebugInfo.cpp

This revision is now accepted and ready to land.Aug 17 2017, 9:24 AM
This revision was automatically updated to reflect the committed changes.