[MergeICmps] Allow sinking past non-load/store

[nikic]

Summary

This is a followup to 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.

[courbet]

Sounds reasonable.

I guess the only caveat that we can only reorder a load/store with non-volatile ones. Removing the check for simplicity of the sinked load/stores looks fine because we are checking that merged loads are simple (=> non-volatile) anyway, so we can indeed reorder the sinked loads with merged loads.

llvm/lib/Transforms/Scalar/MergeICmps.cpp

 #include <vector>
 
 using namespace llvm;
 
 namespace {
 
 #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.
 struct BCEAtom {
   BCEAtom() = default;
   BCEAtom(GetElementPtrInst *GEP, LoadInst *LoadI, int BaseId, APInt Offset)
       : GEP(GEP), LoadI(LoadI), BaseId(BaseId), Offset(Offset) {}
 
private:
   BCECmp Cmp;
 };
 
 bool BCECmpBlock::canSinkBCECmpInst(const Instruction *Inst,
                                     AliasAnalysis &AA) const {
   // 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)) ||
         isModSet(AA.getModRefInfo(Inst, RLoc)))
       return false;
   }
   // Make sure this instruction does not use any of the BCE cmp block
   // instructions as operand.

llvm/test/Transforms/MergeICmps/X86/split-block-does-work.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt < %s -mergeicmps -verify-dom-info -mtriple=x86_64-unknown-unknown -S | FileCheck %s --check-prefix=X86
 
 %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(
 ; X86-LABEL: @opeq1(
 ; X86-NEXT:  "entry+land.rhs.i+land.rhs.i.2+land.rhs.i.3":
 ; X86-NEXT:    call void (...) @foo()
 ; X86-NEXT:    [[TMP0:%.*]] = getelementptr inbounds [[S:%.*]], %S* [[A:%.*]], i64 0, i32 0
 ; X86-NEXT:    [[TMP1:%.*]] = getelementptr inbounds [[S]], %S* [[B:%.*]], i64 0, i32 0