[funcatts] Rewrite callsite operand handling in memory access inference

[reames]

Summary

This is a rewrite of the existing code. It isn't even close to NFC as it fixes several problems with the existing code. Changes detailed below.

1. Argument attributes on an indirect call site are honored. We'd previously only been honoring these on unknown direct calls, which is just weird.
2. Consistently treat calling a function pointer as an operation which reads the function pointer. Previously, we gave it different treatment based on which attributes were on the callsite. (e.g. a readonly callsite vs a writeonly callsite changed the intepretation of the function pointer access for an indirect call)
3. Honor attributes on vararg function declarations when visiting a vararg argument. (Previously, we aborted the entire analysis.)

In addition to the previously described functional changes, the resulting code is both shorter, and easier to follow.

[reames]

Needs reworked over previous patch in stack.

[reames]

I'm starting to split this into individual pieces with the writeonly dependence removed. The first one handles indirect calls, and is posted as D115916.

[reames]

D115961 and D115964 were the next two pieces of this, both have now landed.

The last bit is honoring call site parameter attributes on indirect calls outside the SCC. I'll post that separately and then close this review because everything will have been covered.

[reames]

This has been replaced by a series of smaller patches implementing individual parts.

The last of which - parameter attributes on indirect calls - is out for review as D116118.

clang/test/CodeGen/arm-cmse-attr.c

 void f3()
 {
 }
 
 void f4() __attribute__((cmse_nonsecure_entry))
 {
 }
 
-// CHECK: define{{.*}} void @f1(void ()* nocapture %fptr) {{[^#]*}}#0 {
+// CHECK: define{{.*}} void @f1(void ()* nocapture readonly %fptr) {{[^#]*}}#0 {
 // CHECK: call void %fptr() #2
-// CHECK: define{{.*}} void @f2(void ()* nocapture %fptr) {{[^#]*}}#0 {
+// CHECK: define{{.*}} void @f2(void ()* nocapture readonly %fptr) {{[^#]*}}#0 {
 // CHECK: call void %fptr() #2
 // CHECK: define{{.*}} void @f3() {{[^#]*}}#1 {
 // CHECK: define{{.*}} void @f4() {{[^#]*}}#1 {
 
 // CHECK-NOSE-NOT: cmse_nonsecure_entry
 // CHECK-NOSE-NOT: cmse_nonsecure_call
 // CHECK-SE: attributes #0 = { nounwind
 // CHECK-SE: attributes #1 = { {{.*}} "cmse_nonsecure_entry"
 // CHECK-SE: attributes #2 = { {{.*}} "cmse_nonsecure_call"

llvm/lib/Transforms/IPO/FunctionAttrs.cpp

case Instruction::Invoke: {
       };
 
       CallBase &CB = cast<CallBase>(*I);
       if (CB.doesNotAccessMemory()) {
         AddUsersToWorklistIfCapturing();
         continue;
       }
 
-      Function *F = CB.getCalledFunction();
-      if (!F) {
-        if (CB.onlyReadsMemory()) {
-          IsRead = true;
-          AddUsersToWorklistIfCapturing();
-          continue;
-        } else if (!CB.isCallee(U) && CB.hasFnAttr(Attribute::WriteOnly)) {
-          IsWrite = true;
-          AddUsersToWorklistIfCapturing();
-          continue;
-        }
-        return Attribute::None;
+      if (CB.isCallee(U)) {
+        // Indirect call of an argument is a reading, noncapturing use
+        Captures = false;
       }
 
+      // U cannot be the callee operand use as we explicitly checked for that.
       // Note: the callee and the two successor blocks *follow* the argument
       // operands.  This means there is no need to adjust UseIndex to account
       // for these.
-
-      unsigned UseIndex = std::distance(CB.arg_begin(), U);
-
-      // U cannot be the callee operand use: since we're exploring the
-      // transitive uses of an Argument, having such a use be a callee would
-      // imply the call site is an indirect call or invoke; and we'd take the
-      // early exit above.
+      const unsigned UseIndex = std::distance(CB.arg_begin(), U);
       assert(UseIndex < CB.data_operands_size() &&
              "Data operand use expected!");
 
-      bool IsOperandBundleUse = UseIndex >= CB.arg_size();
-
-      if (UseIndex >= F->arg_size() && !IsOperandBundleUse) {
-        assert(F->isVarArg() && "More params than args in non-varargs call");
-        return Attribute::None;
-      }
-
       Captures &= !CB.doesNotCapture(UseIndex);
 
-      // Since the optimizer (by design) cannot see the data flow corresponding
-      // to a operand bundle use, these cannot participate in the optimistic SCC
-      // analysis.  Instead, we model the operand bundle uses as arguments in
-      // call to a function external to the SCC.
-      if (IsOperandBundleUse ||
-          !SCCNodes.count(&*std::next(F->arg_begin(), UseIndex))) {
-
-        // The accessors used on call site here do the right thing for calls and
-        // invokes with operand bundles.
+      auto canSpeculate = [&]() {
+        if (UseIndex >= CB.arg_size())
+          // Since the optimizer (by design) cannot see the data flow
+          // corresponding to a operand bundle use, these cannot participate in
+          // the optimistic SCC analysis.  Instead, we model the operand bundle
+          // uses as arguments in call to a function external to the SCC.
+          return false;
+        auto *F = CB.getCalledFunction();
+        if (!F)
+          return false;
+        if (UseIndex >= F->arg_size()) {
+          assert(F->isVarArg());
+          return false;
+        }
+        Argument *A = &*std::next(F->arg_begin(), UseIndex);
+        return 0 != SCCNodes.count(A);
+      };
+      if (canSpeculate()) {
+        // Speculatively ignore this use for now.
+        AddUsersToWorklistIfCapturing();
+        continue;
+      }
 
-        if (CB.doesNotAccessMemory(UseIndex)) { /* nop */ }
-        else if (CB.onlyReadsMemory() || CB.onlyReadsMemory(UseIndex))
-          IsRead = true;
-        else if (CB.hasFnAttr(Attribute::WriteOnly) ||
-                 CB.dataOperandHasImpliedAttr(UseIndex, Attribute::WriteOnly))
-          IsWrite = true;
-        else
-          return Attribute::None;
-      }
 
       AddUsersToWorklistIfCapturing();
       break;
     }
 
     case Instruction::Load:
       // A volatile load has side effects beyond what readonly can be relied
       // upon.

llvm/test/Transforms/FunctionAttrs/nocapture.ll

 
 ; FNATTR: define void @nc2(i32* nocapture %p, i32* %q)
 define void @nc2(i32* %p, i32* %q) {
   %1 = call i32 @nc1(i32* %q, i32* %p, i1 0)    ; <i32> [#uses=0]
   ret void
 }
 
 
-; FNATTR: define void @nc3(void ()* nocapture %p)
+; FNATTR: define void @nc3(void ()* nocapture readonly %p)
 define void @nc3(void ()* %p) {
   call void %p()
   ret void
 }
 
 declare void @external(i8*) readonly nounwind
 ; FNATTR: define void @nc4(i8* nocapture readonly %p)
 define void @nc4(i8* %p) {
   call void @external(i8* %p)
   ret void
 }
 
-; FNATTR: define void @nc5(void (i8*)* nocapture %f, i8* nocapture %p)
+; FNATTR: define void @nc5(void (i8*)* nocapture readonly %f, i8* nocapture %p)
 define void @nc5(void (i8*)* %f, i8* %p) {
   call void %f(i8* %p) readonly nounwind
   call void %f(i8* nocapture %p)
   ret void
 }
 
 ; FNATTR:     define void @test1_1(i8* nocapture readnone %x1_1, i8* %y1_1, i1 %c)
 ; It would be acceptable to add readnone to %y1_1 and %y1_2.

llvm/test/Transforms/FunctionAttrs/out-of-bounds-iterator-bug.ll

 ; RUN: opt -function-attrs -S < %s | FileCheck %s
 ; RUN: opt -passes=function-attrs -S < %s | FileCheck %s
 
-; This checks for an iterator wraparound bug in FunctionAttrs.  The previous
-; "incorrect" behavior was inferring readonly for the %x argument in @caller.
-; Inferring readonly for %x *is* actually correct, since @va_func is marked
-; readonly, but FunctionAttrs was inferring readonly for the wrong reasons (and
-; we _need_ the readonly on @va_func to trigger the problematic code path).  It
-; is possible that in the future FunctionAttrs becomes smart enough to infer
-; readonly for %x for the right reasons, and at that point this test will have
-; to be marked invalid.
+; This checks for a previously existing iterator wraparound bug in
+; FunctionAttrs, and in the process covers corner cases with varargs.
 
 declare void @llvm.va_start(i8*)
 declare void @llvm.va_end(i8*)
 
 define void @va_func(i32* readonly %b, ...) readonly nounwind {
 ; CHECK-LABEL: define void @va_func(i32* nocapture readonly %b, ...)
  entry:
   %valist = alloca i8
   call void @llvm.va_start(i8* %valist)
   call void @llvm.va_end(i8* %valist)
   %x = call i32 @caller(i32* %b)
   ret void
 }
 
 define i32 @caller(i32* %x) {
-; CHECK-LABEL: define i32 @caller(i32* nocapture %x)
+; CHECK-LABEL: define i32 @caller(i32* nocapture readonly %x)
  entry:
   call void(i32*,...) @va_func(i32* null, i32 0, i32 0, i32 0, i32* %x)
   ret i32 42
 }
+
+define void @va_func2(i32* readonly %b, ...) {
+; CHECK-LABEL: define void @va_func2(i32* nocapture readonly %b, ...)
+ entry:
+  %valist = alloca i8
+  call void @llvm.va_start(i8* %valist)
+  call void @llvm.va_end(i8* %valist)
+  %x = call i32 @caller(i32* %b)
+  ret void
+}
+
+define i32 @caller2(i32* %x, i32* %y) {
+; CHECK-LABEL: define i32 @caller2(i32* nocapture readonly %x, i32* %y)
+ entry:
+  call void(i32*,...) @va_func2(i32* %x, i32 0, i32 0, i32 0, i32* %y)
+  ret i32 42
+}
+

llvm/test/Transforms/FunctionAttrs/writeonly.ll

 declare void @direct3_callee(i8* writeonly %p)
 
 ; CHECK: define void @direct3(i8* writeonly %p)
 define void @direct3(i8* %p) {
   call void @direct3_callee(i8* %p)
   ret void
 }
 
-; CHECK: define void @fptr_test1(i8* %p, void (i8*)* nocapture %f)
+; CHECK: define void @fptr_test1(i8* %p, void (i8*)* nocapture readonly %f)
 define void @fptr_test1(i8* %p, void (i8*)* %f) {
   call void %f(i8* %p)
   ret void
 }
 
-; CHECK: define void @fptr_test2(i8* %p, void (i8*)* nocapture %f)
+; CHECK: define void @fptr_test2(i8* writeonly %p, void (i8*)* nocapture readonly %f)
 define void @fptr_test2(i8* %p, void (i8*)* %f) {
   call void %f(i8* writeonly %p)
   ret void
 }
 
-; CHECK: define void @fptr_test3(i8* writeonly %p, void (i8*)* nocapture %f)
+; CHECK: define void @fptr_test3(i8* writeonly %p, void (i8*)* nocapture readonly %f)
 define void @fptr_test3(i8* %p, void (i8*)* %f) {
   call void %f(i8* %p) writeonly
   ret void
 }
 
 ; CHECK: attributes #0 = { {{.*}}readnone{{.*}} }
 ; CHECK: attributes #1 = { {{.*}}readonly{{.*}} }
 ; CHECK: attributes #2 = { {{.*}}writeonly{{.*}} }