<div dir="ltr">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><br><div class="gmail_quote"><div dir="ltr">On Thu, Jun 28, 2018 at 7:39 AM Bjorn Pettersson via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">bjope added inline comments.<br>
<br>
<br>
================<br>
Comment at: lib/Transforms/Utils/Local.cpp:1752<br>
+  // Handle compatible pointer <-> integer conversions.<br>
+  return (FromTy->isPointerTy() || ToTy->isPointerTy()) &&<br>
+         !DL.isNonIntegralPointerType(FromTy) &&<br>
----------------<br>
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>
Don't you need to check that the sizes match here? Or in what sense are they compatible?<br>
<br>
<br>
<br>
================<br>
Comment at: lib/Transforms/Utils/Local.cpp:1806<br>
+        // Calculate the high bits and OR them together with the low bits.<br>
+        SmallVector<uint64_t, 8> Ops({dwarf::DW_OP_dup, dwarf::DW_OP_constu,<br>
+                                      (ToBits - 1), dwarf::DW_OP_shr,<br>
----------------<br>
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>
<br>
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>
<br>
<br>
================<br>
Comment at: lib/Transforms/Utils/Local.cpp:1811<br>
+        return DIExpression::prependOpcodes(OldDII.getExpression(), Ops,<br>
+                                            DIExpression::WithStackValue);<br>
+      }<br>
----------------<br>
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>
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>
<br>
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>
<br>
<br>
<br>
<a href="https://reviews.llvm.org/D48676" rel="noreferrer" target="_blank">https://reviews.llvm.org/D48676</a><br>
<br>
<br>
<br>
</blockquote></div></div>