[EarlyCSE] Handle calls with no MemorySSA info.

[gberry]

Summary

When checking for memory dependencies between calls using MemorySSA,
handle cases where the calls have no MemoryAccess associated with them
because the AA analysis being used has determined that the call does not
read/write memory.

Fixes PR33756

[• dberlin]

You may want to not treat these as memory at all, as Vedant suggested on list.

As a further example of the advantages of that approach, i believe
" if ((Inst->mayReadFromMemory() || Inst->mayThrow()) &&

  !(MemInst.isValid() && !MemInst.mayReadFromMemory()))
LastStore = nullptr;"

Will still trigger in your current fix for these calls, but should not.

In fact, it can't read from memory if it has no memory access.

IE you should be able to have a testcase where your current fix avoids eliminating a store, but changing this routine to say "if memoryssa doesn't think it has an access, and it doesn't throw, it's fine", does eliminate the store.

lib/Transforms/Scalar/EarlyCSE.cpp

567 ↗

(On Diff #106266)

you can auto * these

[davide]

This fix is basically correct, I'm fine with this going in after Dan's comments are addressed.
I don't have strong opinions on the alternative solution suggested by Vedant, so up to you.

test/Transforms/EarlyCSE/globalsaa-memoryssa.ll

13–18 ↗

(On Diff #106266)

Consider autogenerating checks using the update-test-checks.py in llvm/utils

[• dberlin]

Yeah, i forgot to say, i'm fine if you just want to fix this as you have already, but if you do, you should leave a note saying it's not optimal or something.

[gberry]

I'm going to go ahead with this version of the fix (with the comments addressed). I implemented the stronger version (i.e. looking at MSSA memory accesses when checking doesNotAccessMemory, onlyReadsMemory, mayReadFromMemory, mayWriteMemory) and saw a total of 2 differences in the values returned from these checks across all of our benchmarks, and no change in code generation at all. Given the amount of code churn to implement this and how close we are to release, I'm going to take the conservative route.

[davide]

In D35317#809975, @gberry wrote:

I'm going to go ahead with this version of the fix (with the comments addressed). I implemented the stronger version (i.e. looking at MSSA memory accesses when checking doesNotAccessMemory, onlyReadsMemory, mayReadFromMemory, mayWriteMemory) and saw a total of 2 differences in the values returned from these checks across all of our benchmarks, and no change in code generation at all. Given the amount of code churn to implement this and how close we are to release, I'm going to take the conservative route.

Fair enough, Geoff.

llvm/trunk/lib/Transforms/Scalar/EarlyCSE.cpp

bool EarlyCSE::isSameMemGeneration(unsigned EarlierGeneration,
                                    Instruction *LaterInst) {
   // Check the simple memory generation tracking first.
   if (EarlierGeneration == LaterGeneration)
     return true;
 
   if (!MSSA)
     return false;
 
+  // If MemorySSA has determined that one of EarlierInst or LaterInst does not
+  // read/write memory, then we can safely return true here.
+  // FIXME: We could be more aggressive when checking doesNotAccessMemory(),
+  // onlyReadsMemory(), mayReadFromMemory(), and mayWriteToMemory() in this pass
+  // by also checking the MemorySSA MemoryAccess on the instruction.  Initial
+  // experiments suggest this isn't worthwhile, at least for C/C++ code compiled
+  // with the default optimization pipeline.
+  auto *EarlierMA = MSSA->getMemoryAccess(EarlierInst);
+  if (!EarlierMA)
+    return true;
+  auto *LaterMA = MSSA->getMemoryAccess(LaterInst);
+  if (!LaterMA)
+    return true;
+
   // Since we know LaterDef dominates LaterInst and EarlierInst dominates
   // LaterInst, if LaterDef dominates EarlierInst then it can't occur between
   // EarlierInst and LaterInst and neither can any other write that potentially
   // clobbers LaterInst.
   MemoryAccess *LaterDef =
       MSSA->getWalker()->getClobberingMemoryAccess(LaterInst);
-  return MSSA->dominates(LaterDef, MSSA->getMemoryAccess(EarlierInst));
+  return MSSA->dominates(LaterDef, EarlierMA);
 }
 
 bool EarlyCSE::processNode(DomTreeNode *Node) {
   bool Changed = false;
   BasicBlock *BB = Node->getBlock();
 
   // If this block has a single predecessor, then the predecessor is the parent
   // of the domtree node and all of the live out memory values are still current

llvm/trunk/test/Transforms/EarlyCSE/globalsaa-memoryssa.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -S -globals-aa -early-cse-memssa | FileCheck %s
+
+define i16 @f1() readonly {
+  ret i16 0
+}
+
+declare void @f2()
+
+; Check that EarlyCSE correctly handles function calls that don't have
+; a MemoryAccess.  In this case the calls to @f1 have no
+; MemoryAccesses since globals-aa determines that @f1 doesn't
+; read/write memory at all.
+
+define void @f3() {
+; CHECK-LABEL: @f3(
+; CHECK-NEXT:    [[CALL1:%.*]] = call i16 @f1()
+; CHECK-NEXT:    call void @f2()
+; CHECK-NEXT:    ret void
+;
+  %call1 = call i16 @f1()
+  call void @f2()
+  %call2 = call i16 @f1()
+  ret void
+}