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

[Philip Reames via Phabricator via llvm-commits]

reames requested changes to this revision.
reames added inline comments.
This revision now requires changes to proceed.


================
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) ||
----------------
>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()


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4456
+    // If this is the first addrmode then everything is fine.
+    if (AddrModes.size() == 0) {
+      AddrModes.emplace_back(NewAddrMode);
----------------
use empty()


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4484
       continue;
 
+    // If NewAddrMode differs in only one dimension and comes from a phi or
----------------
If I understand your original patch correctly, you only support difference in one input across all AddrModes right?  

If so, it might be better to track which fields is known to differ and reject eagerly any that differs in a different field.  Using something like an Optional<FieldName> where FieldName is a enum might be a good representation.

Alternatively, this might make more sense as a post processing loop.  (i.e. keep track of non-equal address modes in a set, then determine if all elements of the set are compatible.)


================
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) {
----------------
We don't have the transform code yet to support this do we?  If not, don't we need to remove this?


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4500
+  // now.
+  if (AddrModes.size() == 0 || (AddrModes.size() > 1 && TrivialAddrMode)) {
     TPT.rollback(LastKnownGood);
----------------
For the moment, you only handle AddrMode.size() == 1.  Just make that your exit condition here and remove the bailout below.

>From this line, it looks like the trivial case might be a change in behaviour for the existing code.  Is that so?  If so, where's the test case for that?


Repository:
  rL LLVM

https://reviews.llvm.org/D38278