[PATCH] D118676: [RS4GC] Extract rematerilazable candidate search. NFC.

[Serguei Katkov via Phabricator via llvm-commits]

skatkov added a comment.

I agree that this re-materialization is not great (especially cost model). Will explain my understanding in a separate comment.



================
Comment at: llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:287
+  SmallVector<Instruction *, 3> ChainToBase;
+  // Original base.
+  Value *RootOfChain;
----------------
reames wrote:
> Shouldn't this always be the base pointer of the last element in the chain?  If so, might be good to either not store explicitly, or at least assert.
According to comment below (special handling of findBasePointer inefficiency) RootOfChain might differ from found base pointer and we need to keep it to make a replacement of RootOfChain during re-materialization.

According to findRematerializationCandidates, RootOfChain  might be only base or alternative phi. This is by construction. What assert do you propose to add? Just to repeat a logic of findRematerializationCandidates?

If we solve the problem with inefficiency of findBasePointer we will need this field at all.


================
Comment at: llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:2275
       // deficiency in the findBasePointer algorithm.
       if (!AreEquivalentPhiNodes(*OrigRootPhi, *AlternateRootPhi))
         continue;
----------------
reames wrote:
> This really looks like something we should fix in the findBasePointer code, but we can leave it for the moment.  We're adding complexity to avoid fixing another problem, and that seems non ideal. 
Agreed. I wanted to keep it as NFC.


================
Comment at: llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:2330
+    // For each live pointer find get its defining chain.
+    SmallVector<Instruction *, 3> ChainToBase = Record.ChainToBase;
+    // Walk backwards to visit top-most instructions first.
----------------
reames wrote:
> You're doing a copy here, but I don't see either the copy or the source modified.  Did you mean this to be a const reference?
Next line is a reverse of the chain.


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

https://reviews.llvm.org/D118676