[MergeICmps] Relax sinking check

[nikic]

Summary

The check for sinking instructions past the load + cmp sequence currently checks for side-effects, which includes writing to memory and unwinding. However, I don't believe we care about sinking the instructions past an unwind (as they don't have any side-effects themselves).

Noticed this will reviewing uses of mayHaveSideEffect() for an upcoming change.

[courbet]

I'm not sure about the nounwind: If the function does raise a exception, we can't know that it's fine to read past the first pair of elements.

[nikic]

In D106591#2899194, @courbet wrote:

I'm not sure about the nounwind: If the function does raise a exception, we can't know that it's fine to read past the first pair of elements.

The transform here is the other way around though: The loads are sunk below the other instructions. So we will do less loads if an unwind occurs, not more. As we checked that the loads are simple, that should be legal.

[courbet]

In D106591#2899221, @nikic wrote:

In D106591#2899194, @courbet wrote:

I'm not sure about the nounwind: If the function does raise a exception, we can't know that it's fine to read past the first pair of elements.

The transform here is the other way around though: The loads are sunk below the other instructions. So we will do less loads if an unwind occurs, not more. As we checked that the loads are simple, that should be legal.

Ah, yes, right. Great !

llvm/lib/Transforms/Scalar/MergeICmps.cpp

private:
   BCEAtom Lhs_;
   BCEAtom Rhs_;
   int SizeBits_ = 0;
 };
 
 bool BCECmpBlock::canSinkBCECmpInst(const Instruction *Inst,
                                     DenseSet<Instruction *> &BlockInsts,
                                     AliasAnalysis &AA) const {
-  // If this instruction has side effects and its in middle of the BCE cmp block
-  // instructions, then bail for now.
-  if (Inst->mayHaveSideEffects()) {
+  // 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
     MemoryLocation LLoc = MemoryLocation::get(Lhs_.LoadI);
     MemoryLocation RLoc = MemoryLocation::get(Rhs_.LoadI);
     if (isModSet(AA.getModRefInfo(Inst, LLoc)) ||
         isModSet(AA.getModRefInfo(Inst, RLoc)))

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(...)  nounwind readnone
+declare void @foo(...) readonly
 
 ; 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