[PATCH] D50585: [WIP][LICM] Just rely on AA rather than special case calls

[Philip Reames via Phabricator via llvm-commits]

reames created this revision.
reames added reviewers: mkazantsev, anna, hfinkel.
Herald added subscribers: llvm-commits, bollu, mcrosier.

Obviously, not ready to check in... I was thinking about the assume and guard patches currently out for review and/or recently landed by Max and I.  Something was really bugging me, and I finally realized what it was.  The code we were adding in LICM was essentially duplicating support already in AliasSetTracker and BasicAA.  We were extending the existing code structure, but it raised the question of whether this was the right approach.

This patch demonstrates that yes, all the special casing can be deleted.  It needs cleanup (and a lot more testing), but it's rather telling there are no test changes observed.  And BasicAA+AST is generally more powerful than the special cases which previously was in LICM.  (I need to find such an example for a test.)

Anyone see a problem with this basic idea?  I'm worried this is too good to be true and I must be missing something...


Repository:
  rL LLVM

https://reviews.llvm.org/D50585

Files:
  include/llvm/Analysis/AliasSetTracker.h
  lib/Transforms/Scalar/LICM.cpp


Index: lib/Transforms/Scalar/LICM.cpp
===================================================================
--- lib/Transforms/Scalar/LICM.cpp
+++ lib/Transforms/Scalar/LICM.cpp
@@ -658,15 +658,16 @@
     if (CI->mayThrow())
       return false;
 
+#if 0
     if (Function *F = CI->getCalledFunction())
         switch (F->getIntrinsicID()) {
         default: break;
         // TODO: support invariant.start, and experimental.guard here
         case Intrinsic::assume:
           // Assumes don't actually alias anything or throw
           return true;
         };
-    
+
     // Handle simple cases by querying alias analysis.
     FunctionModRefBehavior Behavior = AA->getModRefBehavior(CI);
     if (Behavior == FMRB_DoesNotAccessMemory)
@@ -683,17 +684,38 @@
             return false;
         return true;
       }
+      
 
       // If this call only reads from memory and there are no writes to memory
       // in the loop, we can hoist or sink the call as appropriate.
       if (isReadOnly(CurAST))
         return true;
     }
+#endif
 
-    // FIXME: This should use mod/ref information to see if we can hoist or
-    // sink the call.
+    AliasSet *AS = CurAST->findAliasSetForUnknownInst(CI);
+    if (!AS)
+      // Was considered by AST not to alias any possible memory location
+      return true;
+    if (!AS->isMod())
+      // The call (which must be readonly per AST) does read memory clobbered
+      // in the loop.
+      return true;
 
+#if 1
+    // May contain a clobber, give up!
     return false;
+#else
+    // Now check for a uniquely writing instruction within the loop.  In this
+    // case, the value written can't be overwritten or read within the loop.
+    // NOTE: Drop in first patch.
+    auto *UniqueI = AS->getUniqueInstruction();
+    if (!UniqueI)
+      // other memory op, give up
+      return false;
+    assert(UniqueI == CI && "AS must contain FI");
+    return true;
+#endif
   } else if (auto *FI = dyn_cast<FenceInst>(&I)) {
     // Fences alias (most) everything to provide ordering.  For the moment,
     // just give up if there are any other memory operations in the loop.
Index: include/llvm/Analysis/AliasSetTracker.h
===================================================================
--- include/llvm/Analysis/AliasSetTracker.h
+++ include/llvm/Analysis/AliasSetTracker.h
@@ -468,6 +468,7 @@
   /// pointer.
   AliasSet &mergeAllAliasSets();
 
+public: // HACK: fixme
   AliasSet *findAliasSetForUnknownInst(Instruction *Inst);
 };
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D50585.160198.patch
Type: text/x-patch
Size: 2504 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180810/3ae0e9fe/attachment.bin>