[PATCH] RemapValue() assert during type legalisation

[Hal Finkel]

Richard,

I've just opened http://llvm.org/bugs/show_bug.cgi?id=16562 -- and thanks to Duncan for pointing out that this is a duplicate of this bug (and, indeed, fixed by your patch). So now we have a PPC test case too.

+  /// NeWNodes must not appear as targets of ReplacedValues.
(I assume that you meant to write NewNodes not NeWNodes)

FWIW, this does seem reasonable to me. Owen?

 -Hal

----- Original Message -----
> 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
> 
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory