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

[John Brawn via Phabricator via llvm-commits]

john.brawn added inline comments.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:3765
+        auto *TrueValue = CurrentSelect->getTrueValue();
+        assert(Map.find({ TrueValue, CurrentBlock }) != Map.end() &&
+               "No True Value!");
----------------
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).


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


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


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


https://reviews.llvm.org/D36073