[PATCH] D36805: [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.

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added a comment.

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



================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.cpp:829
 
+/// Transfer SDDbgValues.
+static void transferDbgValues(SelectionDAG &DAG, DIBuilder &DIB, SDValue From,
----------------
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.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.cpp:837
+  for (SDDbgValue *Dbg : DAG.GetDbgValues(FromNode)) {
+    if (Dbg->getKind() == SDDbgValue::SDNODE) {
+      auto *Var = cast<DIVariable>(Dbg->getVariable());
----------------
llvm coding style prefers early exit over nesting, i.e: 
```
if (Dbg->getKind() !=SDNode)
   break;
```


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.cpp:838
+    if (Dbg->getKind() == SDDbgValue::SDNODE) {
+      auto *Var = cast<DIVariable>(Dbg->getVariable());
+      DIExpression *Fragment = DIB.createFragmentExpression(
----------------
perhaps change the return type in SDDbgValue instead?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.cpp:842
+          cast_or_null<DIExpression>(Dbg->getExpression()));
+      SDDbgValue *Clone = DAG.getDbgValue(Var, &*Fragment, ToNode,
+                                          To.getResNo(), Dbg->isIndirect(),
----------------
why is the &* necessary?


================
Comment at: llvm/test/DebugInfo/MSP430/sdagsplit-1.ll:20
+; CHECK: DBG_VALUE debug-use {{.*}}, debug-use _, !13, {{.*}}, debug-location !17
+; CHECK: DBG_VALUE debug-use {{.*}}, debug-use _, !13, {{.*}}, debug-location !17
+
----------------
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.


================
Comment at: llvm/test/DebugInfo/MSP430/sdagsplit-1.ll:41
+
+attributes #0 = { nounwind readnone "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #1 = { nounwind readnone speculatable }
----------------
can you remove all non-essential attributes (at least everything in quotes)?


================
Comment at: llvm/test/DebugInfo/X86/sdagsplit-1.ll:18
+; CHECK: .debug_loc
+; CHECK: [[LOC]]:
+; eax, piece 0x00000004, edx, piece 0x00000004
----------------
Probably better to convert this to checking the MIR output, too.


================
Comment at: llvm/test/DebugInfo/X86/sdagsplit-1.ll:41
+
+attributes #0 = { nounwind readnone "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="pentium4" "target-featur
+es"="+fxsr,+mmx,+sse,+sse2,+x87" "unsafe-fp-math"="false" "use-soft-float"="false" }
----------------
ditto


Repository:
  rL LLVM

https://reviews.llvm.org/D36805