[PATCH] D38133: [CGP] Make optimizeMemoryInst introduce a select/phi if it improves things

[Serguei Katkov via Phabricator via llvm-commits]

skatkov added a comment.

Do I understand correctly that you consider only very simple cases?
Specifically if Addr is represented as Select or Phi and no one from its input might be other Select/Phi.

Also it seems you create Phi unconditionally even if the Phi you need already exists.

Please take a look into the patch I come up to cover more cases https://reviews.llvm.org/D36073 but for Phi nodes only and for base registers only.

Does it makes sense for you to review it and extend with select support and other than base parts?



================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4388
+      return nullptr;
+    PHINode *NewPHI = PHINode::Create(T, OrigPHI->getNumIncomingValues(),
+                                      OrigPHI->getName(),
----------------
What if this Phi already exists?


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4581
   // If we saw Phi node then it is not local definitely.
   if (!PhiSeen && none_of(AddrModeInsts, [&](Value *V) {
         return IsNonLocalValue(V, MemoryInst->getParent());
----------------
For select you set PhiSeen to true so you will skip this check but for select all intructions might be in the same basic block. Is it what you expect?
If yes, you need to update a comment.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4617
+      // We can't cope with scale being different
+      if (DifferentScale)
+        return false;
----------------
bail out earlier?


Repository:
  rL LLVM

https://reviews.llvm.org/D38133