[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:
> dblaikie wrote:
> > 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.
> Well, we'd at least pick a line of code that resembled what failed rather than something from another block...
Yep.

Alternatively we could use line zero - more correct, technically, but probably no better for most debuggers/tools (& would make the line tables bigger - coming back to Paul's recent patch/direction/discussion)


https://reviews.llvm.org/D24164