[PATCH] RemapValue() assert during type legalisation

[Hal Finkel]

Richard,

I've found that your patch only fixes some of my test cases hitting this assertion. I've uploaded another three examples to:
http://llvm.org/bugs/show_bug.cgi?id=16562

 -Hal

----- Original Message -----
> 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
> _______________________________________________
> 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