[PATCH] D36073: [CGP] Extends the scope of optimizeMemoryInst optimization

[John Brawn via Phabricator via llvm-commits]

john.brawn accepted this revision.
john.brawn added a comment.

LGTM.



================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:3779
+        auto *CurrentPhi = dyn_cast<PHINode>(CurrentValue);
+        for (auto B : predecessors(CurrentBlock)) {
+          Value *PV = IsDefinedInThisBB
----------------
skatkov wrote:
> john.brawn wrote:
> > The iterator order of predecessors(CurrentBlock) may be different to the iterator order of CurrentPhi->blocks(), which can cause PHI to have incoming values in a different order to CurrentPhi which is a bit of a nuisance when writing tests. Could you see if you can preserve the block order? I tried modifying this to iterate over CurrentPhi->blocks(), but then I hit the "No predecessor Value" assert for reasons I haven't figured out yet.
> We can do this only if CurrentPhi is declared in this basic block otherwise you'll get an assert as you mentioned.
> 
> If we really want it it will require the duplication of the loop to support both cases - in this block and not. Do you see any other possible way to do that?
Looking at this some more it does look like it would be more trouble than it's worth to try and keep the block order, so this I think this is OK as it is.


https://reviews.llvm.org/D36073