[EarlyCSE] Don't DSE across readnone functions that may throw

[sanjoy]

Summary

Depends on D28740

[majnemer]

mayReadFromMemoryOrThrow is a weird predicate, could we just use the other two predicates directly?

[sanjoy]

In D28741#648387, @majnemer wrote:

mayReadFromMemoryOrThrow is a weird predicate, could we just use the other two predicates directly?

Yea, I'll do the same as what I did on the instcombine patch.

[sanjoy]

• address review

[majnemer]

LGTM

lib/Transforms/Scalar/EarlyCSE.cpp

764–766

This comment probably needs a small tweak.

[chandlerc]

LGTM too fwiw.

include/llvm/IR/Instruction.h

public:
 
   /// Return true if this instruction has an AtomicOrdering of unordered or
   /// higher.
   bool isAtomic() const;
 
   /// Return true if this instruction may throw an exception.
   bool mayThrow() const;
 
+  /// Return true if this instruction may throw an exception or read memory.
+  bool mayReadFromMemoryOrThrow() const {
+    return mayReadFromMemory() || mayThrow();
+  }
+
   /// Return true if this instruction behaves like a memory fence: it can load
   /// or store to memory location without being given a memory location.
   bool isFenceLike() const {
     switch (getOpcode()) {
     default:
       return false;
     // This list should be kept in sync with the list in mayWriteToMemory for
     // all opcodes which don't have a memory location.

lib/Transforms/Scalar/EarlyCSE.cpp

if (MemInst.isValid() && MemInst.isLoad()) {
       AvailableLoads.insert(
           MemInst.getPointerOperand(),
           LoadValue(Inst, CurrentGeneration, MemInst.getMatchingId(),
                     MemInst.isAtomic(), MemInst.isInvariantLoad()));
       LastStore = nullptr;
       continue;
     }
 
     // If this instruction may read from memory, forget LastStore.
     // Load/store intrinsics will indicate both a read and a write to
     // memory.  The target may override this (e.g. so that a store intrinsic
     // does not read  from memory, and thus will be treated the same as a
     // regular store for commoning purposes).
-    if (Inst->mayReadFromMemory() &&
+    if (Inst->mayReadFromMemoryOrThrow() &&
         !(MemInst.isValid() && !MemInst.mayReadFromMemory()))
       LastStore = nullptr;
 
     // If this is a read-only call, process it.
     if (CallValue::canHandle(Inst)) {
       // If we have an available version of this call, and if it is the right
       // generation, replace this instruction.
       std::pair<Instruction *, unsigned> InVal = AvailableCalls.lookup(Inst);

test/Transforms/EarlyCSE/readnone-mayunwind.ll

+; RUN: opt -S -early-cse < %s | FileCheck %s
+
+declare void @readnone_may_unwind() readnone
+
+define void @f(i32* %ptr) {
+; CHECK-LABEL: @f(
+; CHECK: store i32 100, i32* %ptr
+; CHECK: call void @readnone_may_unwind()
+; CHECK: store i32 200, i32* %ptr
+
+  store i32 100, i32* %ptr
+  call void @readnone_may_unwind()
+  store i32 200, i32* %ptr
+  ret void
+}