<html><head><meta http-equiv="Content-Type" content="text/html; charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="">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.<div class=""><br class=""></div><div class="">vedant<br class=""><div><br class=""><blockquote type="cite" class=""><div class="">On Jun 28, 2018, at 2:11 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" class="">dblaikie@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class="">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.<br class=""><br class=""><div class="gmail_quote"><div dir="ltr" class="">On Thu, Jun 28, 2018 at 7:39 AM Bjorn Pettersson via Phabricator <<a href="mailto:reviews@reviews.llvm.org" class="">reviews@reviews.llvm.org</a>> wrote:<br class=""></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">bjope added inline comments.<br class="">
<br class="">
<br class="">
================<br class="">
Comment at: lib/Transforms/Utils/Local.cpp:1752<br class="">
+ // Handle compatible pointer <-> integer conversions.<br class="">
+ return (FromTy->isPointerTy() || ToTy->isPointerTy()) &&<br class="">
+ !DL.isNonIntegralPointerType(FromTy) &&<br class="">
----------------<br class="">
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. <br class="">
Don't you need to check that the sizes match here? Or in what sense are they compatible?<br class="">
<br class="">
<br class="">
<br class="">
================<br class="">
Comment at: lib/Transforms/Utils/Local.cpp:1806<br class="">
+ // Calculate the high bits and OR them together with the low bits.<br class="">
+ SmallVector<uint64_t, 8> Ops({dwarf::DW_OP_dup, dwarf::DW_OP_constu,<br class="">
+ (ToBits - 1), dwarf::DW_OP_shr,<br class="">
----------------<br class="">
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).<br class="">
<br class="">
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.<br class="">
<br class="">
<br class="">
================<br class="">
Comment at: lib/Transforms/Utils/Local.cpp:1811<br class="">
+ return DIExpression::prependOpcodes(OldDII.getExpression(), Ops,<br class="">
+ DIExpression::WithStackValue);<br class="">
+ }<br class="">
----------------<br class="">
I don't think WithStackValue is correct if there could be users from findDbgUsers that is a dbg.declare (because dbg.declare is indirect).<br class="">
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?<br class="">
<br class="">
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?<br class="">
<br class="">
<br class="">
<br class="">
<a href="https://reviews.llvm.org/D48676" rel="noreferrer" target="_blank" class="">https://reviews.llvm.org/D48676</a><br class="">
<br class="">
<br class="">
<br class="">
</blockquote></div></div>
</div></blockquote></div><br class=""></div></body></html>