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

[Vedant Kumar via Phabricator via llvm-commits]

vsk planned changes to this revision.
vsk added inline comments.


================
Comment at: lib/Transforms/Utils/Local.cpp:1752
+  // Handle compatible pointer <-> integer conversions.
+  return (FromTy->isPointerTy() || ToTy->isPointerTy()) &&
+         !DL.isNonIntegralPointerType(FromTy) &&
----------------
bjope wrote:
> 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?
> 
I'd like this to be a target-aware way to identify semantically-equivalent bitcasts, inttoptr, and ptrtoint conversions. I'm using isNonIntegralPointerType because, AFAIK, inttoptr/ptrtoint are not lossy on targets with integral pointers. I assumed that the sizes of FromTy and ToTy are equal when at least one of the types is a pointer type, but I'm no longer sure this is a valid assumption. I'll rewrite this to be cleaner and add an explicit check.


================
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,
----------------
bjope wrote:
> 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.
I think LLVM does assume a 64-bit width for the expression stack (at least, we can't represent any DW_OP_constu wider than 64-bits in the IR).

The scheme you describe of splitting up the dbg intrinsics for i64 values and rewriting their expressions sounds very difficult. I'm not familiar with any backends, but IIRC legalization occurs early, so a split i32 may be optimized out leaving you with an incomplete (actually, incorrect) variable description? Does DWARF5 alleviate the issue at all? Maybe you could introduce a vendor extension to apply DW_OP_convert to a subset of a source variable's bits?


================
Comment at: lib/Transforms/Utils/Local.cpp:1811
+        return DIExpression::prependOpcodes(OldDII.getExpression(), Ops,
+                                            DIExpression::WithStackValue);
+      }
----------------
bjope wrote:
> 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?
> 
You're correct to point out that the operand of a dbg.{addr,declare} is the address of a variable, not the variable itself. But I don't see how we'd take the sign-extension code path here when the dbg intrinsic operand is a pointer. Actually, allowing this utility and salvageDI to operate on dbg.addr seems useful, because they can preserve debug info across no-op pointer bitcasts.

I don't think we have any code that would add a DW_OP_stack_value to a dbg.{addr, declare}, but if we did, I don't think it would be weird. IIUC, it would just mean the debugger needs to do pointer arithmetic before loading a variable. That seems just like having a dbg.value for a pointer operand and adding a DW_OP_deref at the end (before DW_OP_stack_value).

I don't understand your last question about dbg.values with non-empty, non-stack-value expressions. It seems reasonable to create these (say, by using DW_OP_deref), and I don't see why they can't also be treated as stack values.


https://reviews.llvm.org/D48676