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

[Nirav Dave via Phabricator via llvm-commits]

niravd marked 5 inline comments as done.
niravd added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:140
     DenseMap<SDNode *, unsigned> WorklistMap;
+    SmallSetVector<SDNode *, 32> PruningList;
 
----------------
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. 


================
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;
+
----------------
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.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:205
 
+    void AddNewNode(SDNode *N) { PruningList.insert(N); }
+
----------------
jyknight wrote:
> Unused?
Yes. Pruned.


================
Comment at: llvm/test/CodeGen/PowerPC/pr39478.ll:10
 ; CHECK:       # %bb.0: # %entry
-; CHECK-NEXT:    lwz 3, 0(3)
-; CHECK-NEXT:    stb 3, 0(3)
 ; CHECK-NEXT:    blr
 entry:
----------------
nemanjai wrote:
> RKSimon wrote:
> > niravd wrote:
> > > RKSimon wrote:
> > > > This looks like an over-reduced test?
> > > With the improved pruning, we are able to narrow the load to 1 byte which as both the load and store are to undef cancel out the both ops. 
> > I'm hitting issues with this test in D58017 as well, but I'd prefer to see it still do what its supposed to (even if its driving me nuts at the moment....)
> > 
> > IMO we need to replace the undef pointers so it still tests https://bugs.llvm.org/show_bug.cgi?id=39478
> I am sorry, I produced this test case by reducing the original input with bugpoint. I can commit an update for it so it doesn't use an undefined pointer as NFC if you'd like so that it stops causing you problems.
That would be great. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58070