[PATCH] D58070: [DAG] Remember nodes added to the worklist for pruning.

[James Y Knight via Phabricator via llvm-commits]

jyknight added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:140
     DenseMap<SDNode *, unsigned> WorklistMap;
+    SmallSetVector<SDNode *, 32> PruningList;
 
----------------
niravd wrote:
> jyknight wrote:
> > Couldn't this just be represented by an index of the last position in the worklist which has been examined for new-node-prunning, instead of a separate list?
> > 
> > Set it to Worklist.size() after popping entries in getNextWorklistEntry(), and iterate from it to the end of worklist in clearNewUnusedWorklistEntries.
> > 
> Unfortunately, that won't catch all nodes. Nodes added to the worklist may already be in the list and therefore won't be put at the end of the list, but we still need to prune those. 
For some reason I had gotten it into my head that this was only important for newly-created nodes, not for all newly-modified nodes, but I see now that's not what this is doing. OK.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:160
+      // Check any nodes added to the worklist to see if they are prunable.
+      SmallSetVector<SDNode *, 16> Nodes;
+
----------------
niravd wrote:
> jyknight wrote:
> > Unused?
> Used at the start of getNextWorklistEntry.  I've have it as a helper, because in principle we need to clear dangling nodes after any failed combine as we may have a created a dangling node that will needlessly impede subsequent combines on the same node and we should call it in those cases. 
> 
> I've added some explanatory comments.
I meant the line this comment was attached to -- the local variable "Nodes" looks unused here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58070/new/

https://reviews.llvm.org/D58070