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

[Vedant Kumar via llvm-commits]

Thanks for pointing that out. I don't think we'll need to match on specific pointee types in this utility, and certainly not for this patch.

vedant

> On Jun 28, 2018, at 2:11 PM, David Blaikie <dblaikie at gmail.com> wrote:
> 
> Side note: please don't depend on what type a pointer points to, if possible (& it should be possible everywhere except where necessary to match the IR  constraints - eg: you may need to check a pointer's pointee type to ensure a use of a value of that type matches up with its definition). Eventually pointee types will go away & someone will have to fix up that code... best to avoid that if we can.
> 
> On Thu, Jun 28, 2018 at 7:39 AM Bjorn Pettersson via Phabricator <reviews at reviews.llvm.org <mailto:reviews at reviews.llvm.org>> wrote:
> 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 <https://reviews.llvm.org/D48676>
> 
> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180628/e1ad8619/attachment.html>