[llvm-commits] [llvm] r62533 - in /llvm/trunk: lib/CodeGen/SelectionDAG/DAGCombiner.cpp lib/CodeGen/SelectionDAG/SelectionDAG.cpp test/CodeGen/X86/pr3018.ll

[Chris Lattner]

On Jan 19, 2009, at 1:44 PM, Dan Gohman wrote:
> URL: http://llvm.org/viewvc/llvm-project?rev=62533&view=rev
> Log:
> Fix SelectionDAG::ReplaceAllUsesWith to behave correctly when
> uses are added to the From node while it is processing From's
> use list, because of automatic local CSE. The fix is to avoid
> visiting any new uses.
>
> Fix a few places in the DAGCombiner that assumed that after
> a RAUW call, the From node has no users and may be deleted.
>
> This fixes PR3018.

Thank you for fixing this Dan!

> +  // Iterate over all the existing uses of From. This specifically  
> avoids
> +  // visiting any new uses of From that arrise while the  
> replacement is
> +  // happening, because any such uses would be the result of CSE:  
> If an
> +  // existing node looks like From after one of its operands is  
> replaced
> +  // by To, we don't want to replace of all its users with To too.
> +  // See PR3018 for more info.

Just from reading this, it isn't clear to me why this is safe.  Is it  
that any new uses get added to the start of the use list?  If so,  
please mention this.  Your implementation of this fix is dramatically  
better than anything I could come up with :)

One other random thing.  The RAUW methods now look like this:

   while (UI != UE) {
     SDNode *U = *UI;
     do ++UI; while (UI != UE && *UI == U);

     // This node is about to morph, remove its old self from the CSE  
maps.
     RemoveNodeFromCSEMaps(U);
     int operandNum = 0;
     for (SDNode::op_iterator I = U->op_begin(), E = U->op_end();
          I != E; ++I, ++operandNum)
       if (I->getVal() == From) {
         From->removeUser(operandNum, U);
         *I = To;
         I->setUser(U);
         To.getNode()->addUser(operandNum, U);
       }

     // Now that we have modified U, add it back to the CSE maps.  If  
it already
     // exists there, recursively merge the results together.
     if (SDNode *Existing = AddNonLeafNodeToCSEMaps(U)) {
...

It seems pretty silly to me to do the op_iterator scan right after  
scanning over the uses like that (from the compile time standpoint).   
Would something like this work:?

   while (UI != UE) {
     SDNode *U = *UI;
     // This node is about to morph, remove its old self from the CSE  
maps.
     RemoveNodeFromCSEMaps(U);

     do {
       unsigned OpNo = UI.getOperandNo();
       ++UI;
       From->removeUser(OpNo, U);
       U->setOperand(OpNo, To);
       To->addUser(OpNo, U);
     } while (UI != UE && *UI == U);

     // Now that we have modified U, add it back to the CSE maps.  If  
it already
     // exists there, recursively merge the results together.
     if (SDNode *Existing = AddNonLeafNodeToCSEMaps(U)) {
...

-Chris