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

[Andrea Di Biagio via llvm-commits]

andreadb 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);
----------------
danielcdh wrote:
> dblaikie wrote:
> > 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)
> So the impact of this patch (or changing it to line 0) will be:
> 
> for debugging/sanitizer, it will make already-bad situation a little worse
> for afdo, it will make a very bad situation much better
> 
> looks like it's still a win?
It indeed looks like a win to me.
As David said, the debug location was already wrong in the first place. To me, this is clearly one of those cases where we should use the line zero. That said, I will let Eric/Paul/David decide as I don't claim to be a debug info experert.


https://reviews.llvm.org/D24164