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

[Serguei Katkov via Phabricator via llvm-commits]

skatkov added inline comments.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:203
 
+static cl::opt<bool> DisableComplexAddrModes(
+    "disable-complex-addr-modes", cl::Hidden, cl::init(false),
----------------
I plan to set this to true before submitting the patch and return back to false as a separate commit.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:209
+static cl::opt<bool>
+AddrSinkNewPhis("addr-sink-new-phis", cl::Hidden, cl::init(false),
+                cl::desc("Allow creation of Phis in Address sinking."));
----------------
If I set this to true I get one unit test failure. The optimization works but generated code for ARM seems worse...
Need to think about some heuristic when to enable Phi node creation. For example if original Phi will be eliminated (no other users except memory operations...)


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:2752
   bool isTrivial() {
     bool Trivial = (BaseGV && BaseGV == OriginalValue) ||
       (BaseReg && BaseReg == OriginalValue);
----------------
Unfortunately it is not enough for checking the trivial case. It is possible that BaseReg is just a bitcast while there is no other fields but OriginalValue != baseReg and we consider this as not trivial what seems not true.

I plan to come up with separate patch for this.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:5044
   // computation.
   Value *&SunkAddr = SunkAddrs[Addr];
   if (SunkAddr) {
----------------
Like an idea for compile time improvement: If we know that for this Addr there is already sunk address why we need all checks above?


https://reviews.llvm.org/D36073