[DAGCombiner] Add node to the worklist in topological order in parallelizeChainedStores

[deadalnix]

Summary

As per title.

[lebedev.ri]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

20665

Outdated comment?

[lebedev.ri]

This seems ok

[deadalnix]

Fix outdated comment

[fhahn]

Can you add a test case, where this triggers?

[deadalnix]

Because the order in which nodes are traversed is not reliable, it is very hard to come up with a test for this. Ultimately, this is part of a larger body of work that intend to ensure nodes are processed in topological order by DAGCombiner. Even if they do not change the codegen, doing these changes are useful as they reduce the pressure on the logic that maintains the topological order.

[RKSimon]

LGTM, I agree that creating a robust test is going to be tricky and probably not worth it.

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

bool DAGCombiner::parallelizeChainedStores(StoreSDNode *St) {
   };
   bool AddNewChain = llvm::all_of(TFOps, hasImprovedChain);
   if (AddNewChain)
     TFOps.insert(TFOps.begin(), NewChain);
 
   SDValue TF = DAG.getTokenFactor(SDLoc(STChain), TFOps);
   CombineTo(St, TF);
 
-  AddToWorklist(STChain);
   // Add TF operands worklist in reverse order.
-  for (auto I = TF->getNumOperands(); I;)
-    AddToWorklist(TF->getOperand(--I).getNode());
   AddToWorklist(TF.getNode());
+  for (const SDValue &Op : TF->ops())
+    AddToWorklist(Op.getNode());
+  AddToWorklist(STChain);
   return true;
 }
 
 bool DAGCombiner::findBetterNeighborChains(StoreSDNode *St) {
   if (OptLevel == CodeGenOpt::None)
     return false;
 
   const BaseIndexOffset BasePtr = BaseIndexOffset::match(St, DAG);