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

[Sergey Dmitrouk via llvm-commits]

sdmitrouk added a comment.

In http://reviews.llvm.org/D12094#226720, @wolfgangp wrote:

> 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.


Not sure about `getNodeIfExists()`, here is part of `DAGCombiner`:

  if (const auto *BinNode = dyn_cast<BinaryWithFlagsSDNode>(N)) {
    CSENode = DAG.getNodeIfExists(N->getOpcode(), N->getVTList(), Ops,
                                  &BinNode->Flags);
  } else {
    CSENode = DAG.getNodeIfExists(N->getOpcode(), N->getVTList(), Ops);
  }

Regarding `FindModifiedNodeSlot()`:

  // See if the modified node already exists.
  void *InsertPos = nullptr;
  if (SDNode *Existing = FindModifiedNodeSlot(N, Op, InsertPos))
    return Existing;

So, if new node already exists, reuse it in different place. Looks like we need to drop old debug location here as well.

I guess the rest are from functions that create constant nodes.

As for the failing test, maybe it's relying on something it shouldn't? In general, cached nodes are queried for reuse, I'm not sure there are other valid cases, that's why I'm a bit suspicious about leaving some cases unhandled.

> 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.


I agree, changing naming shouldn't be a problem, if you have something suitable on your mind. The interface will differ a bit for different queries, but that's not an issue as for me.


http://reviews.llvm.org/D12094