[PATCH] RemapValue() assert during type legalisation

[Richard Sandiford]

In this testcase, lovingly reduced from a large llvmpipe one, we abort
during type legalisation in DAGTypeLegalizer::RemapValue():

void DAGTypeLegalizer::RemapValue(SDValue &N) {
  DenseMap<SDValue, SDValue>::iterator I = ReplacedValues.find(N);
  if (I != ReplacedValues.end()) {
    // Use path compression to speed up future lookups if values get multiply
    // replaced with other values.
    RemapValue(I->second);
    N = I->second;
    assert(N.getNode()->getNodeId() != NewNode && "Mapped to new node!");
  }
}

What seems to be happening is that for one particular node replacement in
DAGTypeLegalizer::ReplaceValueWith():

  DAG.ReplaceAllUsesOfValueWith(From, To);

there are two nodes N1 and N2 whose operands are CSEd, but which themselves
remain unique.  These nodes are marked "new" and are added to the
NodesToAnalyze worklist:

    virtual void NodeUpdated(SDNode *N) {
      // Node updates can mean pretty much anything.  It is possible that an
      // operand was set to something already processed (f.e.) in which case
      // this node could become ready.  Recompute its flags.
      assert(N->getNodeId() != DAGTypeLegalizer::ReadyToProcess &&
             N->getNodeId() != DAGTypeLegalizer::Processed &&
             "Invalid node ID for RAUW deletion!");
      N->setNodeId(DAGTypeLegalizer::NewNode);
      NodesToAnalyze.insert(N);
    }

As it happens, N1 is the target of a ReplacedValues replacement.

Later during this same replacement, there is a node N3 that is CSEd to N4.
N4 is a NewNode at address A, and the previous (deleted) incarnation of
the node at address A is still mapped by ReplacedValues.  So when
NodeUpdateListener::NodeDeleted() sees the N3->N4 replacement,
it indirectly calls ExpungeNode(N4) to flush out the stale data.
However, ExpungeNode calls:

  RemapValue(I->second);

on _all_ entries in ReplacedValues, including the one that maps to N1,
even though that entry is not related to A.  (Which is fair enough.
It can't really know in advance which are related to A and which aren't.)
We then assert because N1 was marked NeWNode before being added to the
worklist, but hasn't been processed yet.

Question is, what's the right fix?  The comment at the top of the file says:

  // The final node obtained by mapping by ReplacedValues is not marked NewNode.

but it also says:

  // Note that these invariants may not hold momentarily when processing a node:

which seems to be the case here.  I'm not sure whether the assert has
really found a bug or not.

The patch below avoids asserting for nodes in the ReplaceValueWith()
worklist, but at the cost of making the worklist a member of
DAGTypeLegalizer.

I made the test force s390x-linux-gnu because the test won't fail on
targets where the vector types are legal.  The test also seems very
sensitive to the order of node reuse.

What do you think?  Is this the right fix, or is the assert expected
to hold even in this situation?

Thanks,
Richard


-------------- next part --------------
A non-text attachment was scrubbed...
Name: remap-value-assert.patch
Type: text/x-patch
Size: 4761 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130604/92b9f81c/attachment.bin>