[PATCH] D27468: When GVN removes a redundant load, it should not modify the debug location of the dominating load.

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb added a comment.

In https://reviews.llvm.org/D27468#614929, @aprantl wrote:

> This makes sense to me. Are there other cases where we should invoke the mergeDebugLoc API that we've been discussing in https://reviews.llvm.org/D26256?


Thanks Adrian.

So far, I didn't find any particular case in GVN which would benefit from that new API..

As a side note: that new API would be useful for r280995 (originally discussed at revision https://reviews.llvm.org/D24164).



================
Comment at: lib/Transforms/Scalar/GVN.cpp:1704
+      // If we have a fully redundant load LI dominated by a single value I,
+      // then don't update the debug location of I.
+      if (LI->getDebugLoc() && ValuesPerBlock.size() != 1)
----------------
aprantl wrote:
> This comment repeats what the code is doing but doesn't really say why this is important. Can you add some of the the rationale from the review here?
Sure. I can add a comment similar to what I wrote in the description.
Something like this maybe:
"If I has debug info, then clearly it should not be modified. However, if I has a null debugloc, then it is still potentially incorrect to propagate LI's debugloc because LI may not post-dominate I".


https://reviews.llvm.org/D27468