[PATCH] D108517: [MergeICmps] Allow sinking past non-load/store

[Nikita Popov via Phabricator via llvm-commits]

nikic created this revision.
nikic added a reviewer: courbet.
Herald added a subscriber: hiraditya.
nikic requested review of this revision.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

This is a followup to D106591 <https://reviews.llvm.org/D106591>. MergeICmps currently only allows sinking the loads past either instructions that don't write to memory at all, or simple loads/stores that don't modify the memory the loads access.

The "simple loads/stores" part of this check doesn't seem necessary to me -- AA isModRef() already accurately models any operation that may clobber the memory. For example, in the adjusted test case the transform is still fine if the call to `@foo()` isn't readonly, but inaccessiblememonly -- in both cases, the call cannot modify the loaded memory.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D108517

Files:
  llvm/lib/Transforms/Scalar/MergeICmps.cpp
  llvm/test/Transforms/MergeICmps/X86/split-block-does-work.ll


Index: llvm/test/Transforms/MergeICmps/X86/split-block-does-work.ll
===================================================================
--- llvm/test/Transforms/MergeICmps/X86/split-block-does-work.ll
+++ llvm/test/Transforms/MergeICmps/X86/split-block-does-work.ll
@@ -3,7 +3,7 @@
 
 %S = type { i32, i32, i32, i32 }
 
-declare void @foo(...) readonly
+declare void @foo(...) inaccessiblememonly
 
 ; We can split %entry and create a memcmp(16 bytes).
 define zeroext i1 @opeq1(
Index: llvm/lib/Transforms/Scalar/MergeICmps.cpp
===================================================================
--- llvm/lib/Transforms/Scalar/MergeICmps.cpp
+++ llvm/lib/Transforms/Scalar/MergeICmps.cpp
@@ -66,15 +66,6 @@
 
 #define DEBUG_TYPE "mergeicmps"
 
-// Returns true if the instruction is a simple load or a simple store
-static bool isSimpleLoadOrStore(const Instruction *I) {
-  if (const LoadInst *LI = dyn_cast<LoadInst>(I))
-    return LI->isSimple();
-  if (const StoreInst *SI = dyn_cast<StoreInst>(I))
-    return SI->isSimple();
-  return false;
-}
-
 // A BCE atom "Binary Compare Expression Atom" represents an integer load
 // that is a constant offset from a base value, e.g. `a` or `o.c` in the example
 // at the top.
@@ -244,10 +235,7 @@
   // If this instruction may clobber the loads and is in middle of the BCE cmp
   // block instructions, then bail for now.
   if (Inst->mayWriteToMemory()) {
-    // Bail if this is not a simple load or store
-    if (!isSimpleLoadOrStore(Inst))
-      return false;
-    // Disallow stores that might alias the BCE operands
+    // Disallow instructions that might modify the BCE operands
     MemoryLocation LLoc = MemoryLocation::get(Cmp.Lhs.LoadI);
     MemoryLocation RLoc = MemoryLocation::get(Cmp.Rhs.LoadI);
     if (isModSet(AA.getModRefInfo(Inst, LLoc)) ||


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D108517.367985.patch
Type: text/x-patch
Size: 1818 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210822/5a7e5c24/attachment.bin>