[PATCH] D12094: Avoid the propagation of debug locations in SelectionDAG via CSE

[Wolfgang Pieb via llvm-commits]

wolfgangp added a comment.

In http://reviews.llvm.org/D12094#226510, @sdmitrouk wrote:

> Hi,
>
> Are there any reasons to implement the check in separate function instead of
>  changing 3-argument overload of `SelectionDAG::FindNodeOrInsertPos` to handle
>  nodes of all kinds (that would require updating its comment a bit)?  If I
>  counted correctly, the changes cover 20 of 27 uses of that overload and it might
>  make sense to keep debug location erasure in one place to make it less
>  error-prone, unless there are reasons not to do this.
>
> Regards,
> Sergey


This is what I intended to do at first, but there are several contexts where the call to FindNodeOrInsertPos() is used as a query rather than with an intent to eliminate common subexpressions (e.g. all 3 versions of FindModifiedNodeSlot(), and getNodeIfExists()). It would be undesirable to possibly null out the debugLoc in those contexts, and indeed one llvm test broke by degrading the line number info when I did it this way.
That's not to say filtering the debugLoc couldn't be centralized but it would probably require some rework of the Constant node handling as well. I'm certainly open to that.

The other reason is more stylistic. I'm uncomfortable with having a function called "FindNodeOrInsertPos()", which is meant to retrieve something, have a significant side effect such as nulling out the debug location, when it's not well-defined inside the function how the result is used.


http://reviews.llvm.org/D12094