[PATCH] D25611: When moving a zext near to its associated load, do not retain the origial debug location.

[Andrea Di Biagio via llvm-commits]

andreadb added a comment.

In https://reviews.llvm.org/D25611#571412, @bjope wrote:

> I'm not an expert on debug info either, but to me the solution sound weird.
>  Isn't it a fact then when having optimisations turned on debugging might get tricky.
>
> What if for example the target does not combine the zext and the load into a zextload. Then there would be a zext with apparently incorrect debug location. I'm not fully convinced that incorrect debug locations will aid the cause of making debugging easier.
>
> Isn't the tricky part optimisations that combines instructions with different debug locations. Only one can be kept, so all such optimisations should choose wisely (if possible).  I'm not sure about the strategy used by DAGCombiner here. Is it always keeping the "outer" instructions debug locations per design, or could it be that when combining a zext and and load it should keep the debug location of the load?
>
> My feeling is that the choice should be made when doing the optimisations that needs to pick one debug location and discard the other. In this scenario CodeGenPrepare only moves instructions.


The problem is that instructions hoisted/sunk by CodeGenPrepare may be speculated. So, preserving the debug location would hurt the debugging experience other than causing problems to sample pgo. Also, the intent of the code in CGP is to subordinate the zext to the load. So, logically the the zero-extend is effectively "part of" the load.

> If something should be done when moving the zext, then I guess it should be dropping the debug information. Isn't inheriting it from the load simply wrong? That would just yield incorrect debug location for the zext in case it isn't combined with the load later.

I don't think that it would yield to incorrect debug location if the zext is not combined. I think Paul already explained why zeroing the debug location might not be the best way to go here. We could zero the debug location, however (as he wrote) the zext is not the "important" part of the statement,  and (more important) we don't want to over-use "line 0" as that has a size cost in the line-table section.


https://reviews.llvm.org/D25611