[EarlyCSE] Allow elimination of redundant writeonly calls

[bkramer]

Summary

A writeonly call behaves like a readnone call that also contains a store
to a global determined by the call arguments. This means we can remove
duplicated calls if there are no intervening stores.

This only works if MemorySSA is enabled as we use it to check if there
are conflicting memory accesses.

With this change CSE can handle things like "cos(x)+cos(x) => 2*cos(x)"
even with -fmath-errno.

Based on D48388 by Brian Homerding!

[nikic]

Can you please pre-commit the test with regenerated check lines?

[fhahn]

llvm/test/Transforms/EarlyCSE/memoryssa.ll

160

It might be good to throw in calls with matching and differing arguments?

[jdoerfert]

Drive by: nounwind and nosync come to mind as being somewhat important here. Also willreturn.

[reames]

A couple notes:

1. writeonly calls can write to their arguments, we should have some test coverage for that case

2. writeonly calls can still throw and infinite-loop. Since you're removing a following call with the same arguments, we can infer this won't happen, but test coverage and comments needed.

Once tests are added, we can take a closer look at the code.

Edit: I failed to state that the basic idea seems reasonable and worthwhile.

[fhahn]

Marking as changes requested as there are outstanding comments.

[fhahn]

Actually mark as Request changes

llvm/lib/Transforms/Scalar/EarlyCSE.cpp

struct CallValue {
   }
 
   static bool canHandle(Instruction *Inst) {
     // Don't value number anything that returns void.
     if (Inst->getType()->isVoidTy())
       return false;
 
     CallInst *CI = dyn_cast<CallInst>(Inst);
-    if (!CI || !CI->onlyReadsMemory())
+    // Handle readnone, readonly and writeonly.
+    if (!CI || !(CI->doesNotReadMemory() || CI->onlyReadsMemory()))
       return false;
     return true;
   }
 };
 
 } // end anonymous namespace
 
 namespace llvm {
for (Instruction &Inst : make_early_inc_range(BB->getInstList())) {
     // 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() || Inst.mayThrow()) &&
         !(MemInst.isValid() && !MemInst.mayReadFromMemory()))
       LastStore = nullptr;
 
-    // If this is a read-only call, process it.
+    // If this is a read-only or write-ony call, process it.
     if (CallValue::canHandle(&Inst)) {
+      // Write-only calls can be CSE'd. Increment the generation to prevent
+      // replacement if there are intervening stores.
+      auto PreStoreGeneration = CurrentGeneration;
+      if (Inst.mayWriteToMemory()) {
+        assert(!Inst.mayReadFromMemory());
+        ++CurrentGeneration;
+        LastStore = nullptr;
+      }
+
       // 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);
       if (InVal.first != nullptr &&
-          isSameMemGeneration(InVal.second, CurrentGeneration, InVal.first,
+          isSameMemGeneration(InVal.second, PreStoreGeneration, InVal.first,
                               &Inst)) {
         LLVM_DEBUG(dbgs() << "EarlyCSE CSE CALL: " << Inst
                           << "  to: " << *InVal.first << '\n');
         if (!DebugCounter::shouldExecute(CSECounter)) {
           LLVM_DEBUG(dbgs() << "Skipping due to debug counter\n");
           continue;
         }
         if (!Inst.use_empty())
           Inst.replaceAllUsesWith(InVal.first);
         salvageKnowledge(&Inst, &AC);
         removeMSSA(Inst);
         Inst.eraseFromParent();
         Changed = true;
         ++NumCSECall;
         continue;
       }
 
       // Otherwise, remember that we have this instruction.
-      AvailableCalls.insert(&Inst, std::make_pair(&Inst, CurrentGeneration));
+      AvailableCalls.insert(&Inst, std::make_pair(&Inst, PreStoreGeneration));
       continue;
     }
 
     // A release fence requires that all stores complete before it, but does
     // not prevent the reordering of following loads 'before' the fence.  As a
     // result, we don't need to consider it as writing to memory and don't need
     // to advance the generation.  We do need to prevent DSE across the fence,
     // but that's handled above.

llvm/test/Transforms/EarlyCSE/memoryssa.ll

entry:
   ; CHECK: store i32 %qv, i32* %q
   ; CHECK-NOMEMSSA-LABEL: store i32 %qv, i32* %q
   store i32 %qv, i32* %q
   ret void
 }
 
 declare void @llvm.lifetime.end.p0i8(i64, i32*)
 declare void @llvm.lifetime.start.p0i8(i64, i32*)
+
+;; Check that writeonly call gets CSE'd when using MSSA
+declare i32 @write_only() writeonly
+
+; CHECK-LABEL: @test_write_only
+; CHECK-NOMEMSSA-LABEL: @test_write_only
+define void @test_write_only(i1 %c, i32* %p) {
+; CHECK: call i32 @write_only()
+; CHECK-NOMEMSSA: call i32 @write_only()
+  %call = call i32 @write_only()
+; CHECK: %v1 = load i32, i32* @G1
+; CHECK-NOMEMSSA: %v1 = load i32, i32* @G1
+  %v1 = load i32, i32* @G1
+; CHECK-NOT: call i32 @write_only()
+; CHECK-NOMEMSSA: call i32 @write_only()
+  %call2 =  call i32 @write_only()
+; CHECK: store
+; CHECK-NOMEMSSA: store
+  store i32 %v1, i32* @G2
+; CHECK: call i32 @write_only()
+; CHECK-NOMEMSSA: call i32 @write_only()
+  %call3 = call i32 @write_only()
+; CHECK: store
+; CHECK-NOMEMSSA: store
+  store i32 %v1, i32* @G2
+; CHECK: call i32 @write_only()
+; CHECK-NOMEMSSA: call i32 @write_only()
+  %call4 = call i32 @write_only()
+  ret void
+}