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

[David Blaikie via llvm-commits]

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> 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
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180628/111ff4c5/attachment.html>