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

[Serguei Katkov via Phabricator via llvm-commits]

skatkov marked 2 inline comments as done.
skatkov added inline comments.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:3765
+        auto *TrueValue = CurrentSelect->getTrueValue();
+        assert(Map.find({ TrueValue, CurrentBlock }) != Map.end() &&
+               "No True Value!");
----------------
john.brawn wrote:
> If TrueValue is not an Instruction then this will fail because the value will have been inserted into the Map with nullptr for the block. I think we need handling similar to how PHINode handles it below (and the same is true for FalseValue also).
Good catch. Fixed.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:3779
+        auto *CurrentPhi = dyn_cast<PHINode>(CurrentValue);
+        for (auto B : predecessors(CurrentBlock)) {
+          Value *PV = IsDefinedInThisBB
----------------
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?


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:3816
+      // if it is already visited or it is an ending value then skip it.
+      if (Map.count(Current))
+        continue;
----------------
john.brawn wrote:
> Elsewhere Map.find(Current) != Map.end() is used to see if a value is in the map. It would be nice for that to be checked the same way everywhere.
Done.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:3835
+                                       &CurrentBlock->front());
+        Map.insert({ Current, PHI });
+        NewPhiNodes.insert(PHI);
----------------
john.brawn wrote:
> Map[Current] = PHI (and similar elsewhere in this function).
Done,


https://reviews.llvm.org/D36073