[PATCH] D38278: [CGP] Make optimizeMemoryInst capable of handling multiple AddrModes

[Serguei Katkov via Phabricator via llvm-commits]

skatkov added inline comments.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4447
+    // value are trivial. We need to detect these to avoid introducing a phi or
+    // select which just duplicates what's already there.
+    if ((!NewAddrMode.BaseReg && !NewAddrMode.BaseGV) ||
----------------
reames wrote:
> From the comment, this really sounds like it's covering up a deficiency in the phi/select insertion logic.  Would you mind leaving this case out for the moment or showing a test case where this is essential?  In particular, Serguei's patch goes to lengths to try to reuse existing phi nodes.
> 
> When this is needed, this sounds like a function on AddressMode called isTrivial()
I think it is about earlier bail out and compile time saving.

TrivialAddrMode actually means that if we found that all AddrModes are of the form only base reg or gv (no scale or offset) then later trying to find a common Phi node for, for example, base reg will end up with exactly the same Phi node we started from (addr). Yes my algorithm will detect it and will not create new Phi nodes but it will return the same Phi node addr. So all complex work in my patch is redundant.

This check just detect it in advance and bail out earlier.

I agree it should be moved to AddrMode class.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4487
+    // select then we can handle it by inserting a phi/select later on.
+    if ((isa<PHINode>(Addr) || isa<SelectInst>(Addr)) && Addr->hasOneUse() &&
+        DifferCount == 1) {
----------------
reames wrote:
> We don't have the transform code yet to support this do we?  If not, don't we need to remove this?
I consider this is as a preparation to implementation. For now we allow processing later but will bail out after that.

Not sure that Addr->hasOneUse is a correct check here...

What if we have two instruction which can fold Addr after our modification?


Repository:
  rL LLVM

https://reviews.llvm.org/D38278