[LICM] Hoist calls to readonly argmemonly functions even with stores in the loop

[reames]

Summary

We know that an argmemonly function can only access memory pointed to by it's arguments. Rather than needing to consider all possible stores as aliasing (as we do for a readonly function), we can only consider the aliasing of the pointer arguments.

Note that this change only addresses hoisting. I'm thinking about how to address speculation safety as well, but that will be a different change.

[majnemer]

Can't a readonly, argmemonly function trap?

[reames]

In D12771#243656, @majnemer wrote:

Can't a readonly, argmemonly function trap?

Absolutely. But that's not an aliasing property. That's handled by isGuaranteedToExecute.

[reames]

ping

[sanjoy]

The code looks pretty straightforward, so I'm okay giving an LGTM; but given that I'm not specifically familiar with LICM, it'd be understandable if you want to wait for some more feedback.

[reames]

Hal, David,

Could one of you take a quick look?

Philip

[majnemer]

lib/Transforms/Scalar/LICM.cpp

468–471 ↗

(On Diff #34481)

Is the function permitted to bitcast it's non-pointer parameters and store to them?

[majnemer]

LGTM

lib/Transforms/Scalar/LICM.cpp

468–471 ↗

(On Diff #34481)

Ah, the langref answers this question...

[sanjoy]

lib/Transforms/Scalar/LICM.cpp

468–471 ↗

(On Diff #34481)

I had the exact same question earlier. :) Turns out, the specification for argmemonly forbids that -- an argmemonly function can only store through *pointer typed* arguments.

"This attribute indicates that the only memory accesses inside function are loads and stores from objects pointed to by its pointer-typed arguments, with arbitrary offsets."

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

if (LoadInst *LI = dyn_cast<LoadInst>(&I)) {
     if (isa<DbgInfoIntrinsic>(I))
       return false;
 
     // Handle simple cases by querying alias analysis.
     FunctionModRefBehavior Behavior = AA->getModRefBehavior(CI);
     if (Behavior == FMRB_DoesNotAccessMemory)
       return true;
     if (AliasAnalysis::onlyReadsMemory(Behavior)) {
+      // A readonly argmemonly function only reads from memory pointed to by
+      // it's arguments with arbitrary offsets.  If we can prove there are no
+      // writes to this memory in the loop, we can hoist or sink.
+      if (AliasAnalysis::onlyAccessesArgPointees(Behavior)) {
+        for (Value *Op : CI->arg_operands())
+          if (Op->getType()->isPointerTy() &&
+              pointerInvalidatedByLoop(Op, MemoryLocation::UnknownSize,
+                                       AAMDNodes(), CurAST))
+            return false;
+        return true;
+      }
       // If this call only reads from memory and there are no writes to memory
       // in the loop, we can hoist or sink the call as appropriate.
       bool FoundMod = false;
       for (AliasSetTracker::iterator I = CurAST->begin(), E = CurAST->end();
            I != E; ++I) {
         AliasSet &AS = *I;
         if (!AS.isForwardingAliasSet() && AS.isMod()) {
           FoundMod = true;

llvm/trunk/test/Transforms/LICM/argmemonly-call.ll

+; RUN: opt -S -basicaa -licm %s | FileCheck %s
+declare i32 @foo() readonly argmemonly nounwind
+declare i32 @foo2() readonly nounwind
+declare i32 @bar(i32* %loc2) readonly argmemonly nounwind
+
+define void @test(i32* %loc) {
+; CHECK-LABEL: @test
+; CHECK: @foo
+; CHECK-LABEL: loop:
+  br label %loop
+
+loop:
+  %res = call i32 @foo()
+  store i32 %res, i32* %loc
+  br label %loop
+}
+
+; Negative test: show argmemonly is required
+define void @test_neg(i32* %loc) {
+; CHECK-LABEL: @test_neg
+; CHECK-LABEL: loop:
+; CHECK: @foo
+  br label %loop
+
+loop:
+  %res = call i32 @foo2()
+  store i32 %res, i32* %loc
+  br label %loop
+}
+
+define void @test2(i32* noalias %loc, i32* noalias %loc2) {
+; CHECK-LABEL: @test2
+; CHECK: @bar
+; CHECK-LABEL: loop:
+  br label %loop
+
+loop:
+  %res = call i32 @bar(i32* %loc2)
+  store i32 %res, i32* %loc
+  br label %loop
+}
+
+; Negative test: %might clobber gep
+define void @test3(i32* %loc) {
+; CHECK-LABEL: @test3
+; CHECK-LABEL: loop:
+; CHECK: @bar
+  br label %loop
+
+loop:
+  %res = call i32 @bar(i32* %loc)
+  %gep = getelementptr i32, i32 *%loc, i64 1000000
+  store i32 %res, i32* %gep
+  br label %loop
+}
+
+
+; Negative test: %loc might alias %loc2
+define void @test4(i32* %loc, i32* %loc2) {
+; CHECK-LABEL: @test4
+; CHECK-LABEL: loop:
+; CHECK: @bar
+  br label %loop
+
+loop:
+  %res = call i32 @bar(i32* %loc2)
+  store i32 %res, i32* %loc
+  br label %loop
+}