[EarlyCSE] Understand WriteOnly function calls in EarlyCSE with MSSA

[homerdin]

Summary

I am working to expand the coverage of the WriteOnly attribute when applied to functions:

This patch enables earlyCSE to handle calls to WriteOnly functions and updates lastStore and CurrentGeneration variables appropriately. canHandle will return true for WriteOnly function calls whether we have MSSA or not, but for non-MSSA they will never be the same generation (isSameMemGeneration()) and will not be CSE'd.

Add write_only_func attribute to Clang: https://reviews.llvm.org/D46313
Mark libm functions write_only_func when we care about errno: https://reviews.llvm.org/D48386
Infer WriteOnly attribute for functions: https://reviews.llvm.org/D48387

This patch allows EarlyCSE to handle WriteOnly function calls when using MSSA. Without MSSA the WriteOnly call instructions will be tracked, but never qualify to be eliminated.

Any feedback would be appreciated!

[xbolva00]

lib/Transforms/Scalar/EarlyCSE.cpp

1037

Avoid two dyn_cast?

auto *CI = dyn_cast...
if (CI && CI->doesNotReadMemory() && ...)

[homerdin]

Avoiding two dyn_cast.

Is this condition clear, or would it be useful to extend the helper functions to something like CI->onlyWritesMemory()?

[xbolva00]

Yes, a helper function is a good idea.

[hfinkel]

Makes sense to me. Obviously needs some test.

lib/Transforms/Scalar/EarlyCSE.cpp

1016

Indentation is off (tabs?).

[homerdin]

Fixed indentation, added a test and expanded the summary to explain the difference between early-cse and early-cse-memssa.

[homerdin]

@xbolva00
I ended up not adding a helper function after looking at the existing ones because it won't simplify the code. To follow the current pattern for the helper functions:

OnlyReadsMemory() = ReadNone || ReadOnly
so I could add OnlyWritesMemory() = ReadNone || WriteOnly which is the exact same functionality as doesNotReadMemory()
Either way we would still need the !CI->doesNotAccessMemory.

[xbolva00]

Re-ping.

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 (BasicBlock::iterator I = BB->begin(), E = BB->end(); I != E;) {
     // 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 writeonly 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);
       if (InVal.first != nullptr &&
           isSameMemGeneration(InVal.second, CurrentGeneration, 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;
         }
+        // When handling writeonly function calls we need to update lastStore
+        // and currentGen.
+        auto *CI = dyn_cast<CallInst>(Inst);
+        if (CI && CI->doesNotReadMemory() && !CI->doesNotAccessMemory()) {
+          ++CurrentGeneration;
+          LastStore = nullptr;
+        }
         if (!Inst->use_empty())
           Inst->replaceAllUsesWith(InVal.first);
         removeMSSA(Inst);
         Inst->eraseFromParent();
         Changed = true;
         ++NumCSECall;
         continue;
       }
-
       // Otherwise, remember that we have this instruction.
       AvailableCalls.insert(
           Inst, std::pair<Instruction *, unsigned>(Inst, CurrentGeneration));
+
+      // When handling writeonly function calls we need to update lastStore
+      // and currentGen.
+      auto *CI = dyn_cast<CallInst>(Inst);
+      if (CI && CI->doesNotReadMemory() && !CI->doesNotAccessMemory()) {
+        ++CurrentGeneration;
+        LastStore = nullptr;
+      }
+
       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.

test/Transforms/EarlyCSE/memoryssa.ll

 ; CHECK-NOMEMSSA-LABEL: end:
 end:
 ; CHECK-NOT: load
 ; CHECK-NOMEMSSA: load
   %v2 = load i32, i32* @G1
   store i32 %v2, i32* @G3
   ret void
 }
+
+;; Check that writeonly call gets CSE'd when using MSSA
+declare i32 @write_only() writeonly
+
+define void @test_write_only(i1 %c, i32* %p) {
+; CHECK-LABEL: entry:
+; CHECK-NOMEMSSA-LABEL: entry:
+entry:
+; 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
+}