[PATCH] D59564: [LICM & MemorySSA] Don't sink/hoist stores in the presence of ordered loads.

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea created this revision.
asbirlea added a reviewer: george.burgess.iv.
Herald added subscribers: Prazek, jlebar, sanjoy.
Herald added a project: LLVM.

Before this patch, if any Use existed in the loop, with a defining
access in the loop, we conservatively decide to not move the store.
What this approach was missing, is that ordered loads are not Uses, they're Defs
in MemorySSA. So, even when the clobbering walker does not find that
volatile load to interfere, we still cannot hoist a store past a
volatile load.
Resolves PR41140.


Repository:
  rL LLVM

https://reviews.llvm.org/D59564

Files:
  lib/Transforms/Scalar/LICM.cpp
  test/Transforms/LICM/hoist-debuginvariant.ll


Index: test/Transforms/LICM/hoist-debuginvariant.ll
===================================================================
--- test/Transforms/LICM/hoist-debuginvariant.ll
+++ test/Transforms/LICM/hoist-debuginvariant.ll
@@ -1,6 +1,6 @@
 ; RUN: opt < %s -licm -S | FileCheck %s
 ; RUN: opt < %s -strip-debug -licm -S | FileCheck %s
-; RUN: opt < %s -licm -enable-mssa-loop-dependency=true -verify-memoryssa -S | FileCheck %s --check-prefixes=CHECK,MSSA
+; RUN: opt < %s -licm -enable-mssa-loop-dependency=true -verify-memoryssa -S | FileCheck %s --check-prefixes=CHECK
 
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
@@ -17,7 +17,6 @@
 ; CHECK-NEXT: [[_TMP2:%.*]] = load i32, i32* @a, align 4
 ; CHECK-NEXT: [[_TMP3:%.*]] = load i32, i32* @b, align 4
 ; CHECK-NEXT: [[_TMP4:%.*]] = sdiv i32 [[_TMP2]], [[_TMP3]]
-; MSSA-NEXT: store i32 [[_TMP4:%.*]], i32* @c, align 4
 ; CHECK-NEXT: br label [[BB3:%.*]]
 
   br label %bb3
Index: lib/Transforms/Scalar/LICM.cpp
===================================================================
--- lib/Transforms/Scalar/LICM.cpp
+++ lib/Transforms/Scalar/LICM.cpp
@@ -1191,30 +1191,39 @@
     } else { // MSSAU
       if (isOnlyMemoryAccess(SI, CurLoop, MSSAU))
         return true;
+      // If there are more accesses than the Promotion cap, give up, we're not
+      // walking a list that long.
+      if (NoOfMemAccTooLarge)
+        return false;
+      // Check store only if there's still "quota" to check clobber.
       if (*LicmMssaOptCounter < LicmMssaOptCap) {
-        auto *Source = MSSA->getSkipSelfWalker()->getClobberingMemoryAccess(SI);
-        (*LicmMssaOptCounter)++;
-        // If there are no clobbering Defs in the loop, we still need to check
-        // for interfering Uses. If there are more accesses than the Promotion
-        // cap, give up, we're not walking a list that long. Otherwise, walk the
-        // list, check each Use if it's optimized to an access outside the loop.
-        // If yes, store is safe to hoist. This is fairly restrictive, but
-        // conservatively correct.
+        // If there are interfering Uses (i.e. their defining access is in the
+        // loop), or ordered loads (stored as Defs!), don't move this store.
+        // Could do better here, but this is conservatively correct.
         // TODO: Cache set of Uses on the first walk in runOnLoop, update when
         // moving accesses. Can also extend to dominating uses.
-        if ((!MSSA->isLiveOnEntryDef(Source) &&
-             CurLoop->contains(Source->getBlock())) ||
-            NoOfMemAccTooLarge)
-          return false;
         for (auto *BB : CurLoop->getBlocks())
           if (auto *Accesses = MSSA->getBlockAccesses(BB))
-            for (const auto &MA : *Accesses)
+            for (const auto &MA : *Accesses) {
               if (const auto *MU = dyn_cast<MemoryUse>(&MA)) {
                 auto *MD = MU->getDefiningAccess();
                 if (!MSSA->isLiveOnEntryDef(MD) &&
                     CurLoop->contains(MD->getBlock()))
                   return false;
+              } else {
+                if (const auto *MD = dyn_cast<MemoryDef>(&MA))
+                  if (auto *LI= dyn_cast<LoadInst>(MD->getMemoryInst()))
+                    if (!LI->isUnordered())
+                      return false;
               }
+            }
+
+        auto *Source = MSSA->getSkipSelfWalker()->getClobberingMemoryAccess(SI);
+        (*LicmMssaOptCounter)++;
+        // If there are no clobbering Defs in the loop, store is safe to hoist.
+        if (!MSSA->isLiveOnEntryDef(Source) &&
+             CurLoop->contains(Source->getBlock()))
+          return false;
         return true;
       }
       return false;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D59564.191397.patch
Type: text/x-patch
Size: 3766 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190319/1ece1b56/attachment.bin>