[PATCH] D48676: [Local] Teach insertReplacementDbgValues basic integer/pointer conversions

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added inline comments.


================
Comment at: lib/Transforms/Utils/Local.cpp:1752
+  // Handle compatible pointer <-> integer conversions.
+  return (FromTy->isPointerTy() || ToTy->isPointerTy()) &&
+         !DL.isNonIntegralPointerType(FromTy) &&
----------------
Not sure that I understand this. You say that if at least one of the types is a pointer, and neither is a non-integral pointer type, then they are compatible. 
Don't you need to check that the sizes match here? Or in what sense are they compatible?



================
Comment at: lib/Transforms/Utils/Local.cpp:1806
+        // Calculate the high bits and OR them together with the low bits.
+        SmallVector<uint64_t, 8> Ops({dwarf::DW_OP_dup, dwarf::DW_OP_constu,
+                                      (ToBits - 1), dwarf::DW_OP_shr,
----------------
Some day I think we need to implement support for the types expression stack from DWARF 5. Currently I think LLVM assumes that the DWARF expression stack is 64 bits wide (or in LLVM IR we maybe assume that we have infinite bits and can do DW_OP_shr etc on any type).

For our target the expression stack is 32 bits. And when legalizing larger types like i64 at ISel we end up trying to split these dbg.intrinsics into several DBG_VALUE instruction, using DW_OP_LLVM_fragment for to identify the different parts. Right now we have no simple way in rewriting a complex DIExpression (using 64-bit expressions) into smaller pieces, so we need to discard the debug info.


================
Comment at: lib/Transforms/Utils/Local.cpp:1811
+        return DIExpression::prependOpcodes(OldDII.getExpression(), Ops,
+                                            DIExpression::WithStackValue);
+      }
----------------
I don't think WithStackValue is correct if there could be users from findDbgUsers that is a dbg.declare (because dbg.declare is indirect).
Maybe unlikely to happen here, but I don't know. The same kind of fault exist in salvageDebugInfo, and I've been trying to understand if we ever expect to have non-empty DIExpressions in dbg.declare (except for DW_OP_LLVM_fragment). For example having a DW_OP_stack_value in dbg.declare seems weird, right?

And what happens if we have a dbg.value and the DIExpression already got a non-empty DIExpression, ending without a DW_OP_stack_value. Then it is indirect, and adding DW_OP_stack_value would turn it into being direct. Or should it perhaps be illegal to have an indirect dbg.value like that?



https://reviews.llvm.org/D48676