[PATCH] D24164: Remove debug info when hoisting instruction from then/else branch.

[David Blaikie via llvm-commits]

dblaikie added inline comments.

================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:1248
@@ -1247,1 +1247,3 @@
 
+    if (!isa<CallInst>(I1) &&  I1->getDebugLoc() != I2->getDebugLoc())
+      I1->setDebugLoc(nullptr);
----------------
echristo wrote:
> echristo wrote:
> > I think you have a use after free here, IS->eraseFromParent() above.
> This needs a comment about the point of what we're doing.
> 
> That said while this may help the AFDO type of use case I think it might the debug experience worse for sanitizers/debuggers. For the easy case, take one of the instructions that's being hoisted and make it a load of NULL. If we assert/trap/etc on this then the back trace will give us a fairly unhelpful line in the basic block that doesn't correlate to any of the source that the user wrote.
That's my concern as well - but conversely, even if we did pick a location for this, half the time we'd pick the wrong one which would also be confusing for the user (we could diagnose a null dereference that we would describe as coming from the 'if' block, even though the 'else' was taken - the user could easily see a contradiction here that may be quite confusing).

So, dunno what the right call is - just has some complications.


https://reviews.llvm.org/D24164