[llvm] r214673 - [x86] Don't add nodes to the combined set (and prune subsequent

[Duncan P. N. Exon Smith]

> On 2014-Aug-03, at 16:10, Chandler Carruth <chandlerc at gmail.com> wrote:
> 
> Author: chandlerc
> Date: Sun Aug  3 18:10:59 2014
> New Revision: 214673
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=214673&view=rev
> Log:
> [x86] Don't add nodes to the combined set (and prune subsequent
> combines) until they are legal.
> 
> Doing it the old way could, when the stars align *just* right, cause
> a node to get into the combine set prior to being legalized. Then, when
> the same node showed up as an operand to another node later on (but not
> so much later on that it had been deleted as dead) we would fail to add
> it back to the worklist thinking it had already been combined. This
> would in turn cause it to not be legalized. Fortunately, we can also
> walk the operands looking for uncombined (and thus potentially
> un-legalized) nodes late. It will still ensure that we walk all operands
> of all nodes and send all of them through both the legalizer without
> changes and the combiner at least once. (Which was the original goal of
> this).
> 
> I have a test case for this bug, but it is terribly brittle.

Is there a useful comment you can add to the code?  Maybe:

    // Legalize nodes before adding them to the combined set.

> For
> example, it will stop finding the bug the moment I enable the new
> shuffle lowering. I don't yet have any test case that reliably exercises
> this bug, and it isn't clear that it will be possible to craft one. It
> is entirely possible that with the new shuffle lowering the two forms of
> doing this are precisely equivalent. That doesn't mean we shouldn't take
> the more conservative approach of insisting on things in the combined
> set having survived the legalizer.
> 
> Modified:
>    llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
> 
> Modified: llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp?rev=214673&r1=214672&r2=214673&view=diff
> ==============================================================================
> --- llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp (original)
> +++ llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp Sun Aug  3 18:10:59 2014
> @@ -1152,14 +1152,6 @@ void DAGCombiner::Run(CombineLevel AtLev
>     if (recursivelyDeleteUnusedNodes(N))
>       continue;
> 
> -    // Add any operands of the new node which have not yet been combined to the
> -    // worklist as well. Because the worklist uniques things already, this
> -    // won't repeatedly process the same operand.
> -    CombinedNodes.insert(N);
> -    for (unsigned i = 0, e = N->getNumOperands(); i != e; ++i)
> -      if (!CombinedNodes.count(N->getOperand(i).getNode()))
> -        AddToWorklist(N->getOperand(i).getNode());
> -
>     WorklistRemover DeadNodes(*this);
> 
>     // If this combine is running after legalizing the DAG, re-legalize any
> @@ -1178,6 +1170,14 @@ void DAGCombiner::Run(CombineLevel AtLev
> 
>     DEBUG(dbgs() << "\nCombining: "; N->dump(&DAG));
> 
> +    // Add any operands of the new node which have not yet been combined to the
> +    // worklist as well. Because the worklist uniques things already, this
> +    // won't repeatedly process the same operand.
> +    CombinedNodes.insert(N);
> +    for (unsigned i = 0, e = N->getNumOperands(); i != e; ++i)
> +      if (!CombinedNodes.count(N->getOperand(i).getNode()))
> +        AddToWorklist(N->getOperand(i).getNode());
> +
>     SDValue RV = combine(N);
> 
>     if (!RV.getNode())
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits