[funcattrs] Infer writeonly argument attribute [part 2]

[reames]

Summary

This builds on the code from D114963, and extends it to handle calls both direct and indirect. With the revised code structure (from series of previously landed NFCs), this is pretty straight forward.

One thing to note is that we can not infer writeonly for arguments which might be captured. If the pointer can be read back by the caller, and then read through, we have no way to track that. This is the same restriction we have for readonly, except that we get no mileage out of the "callee can be readonly" exception since a writeonly param on a readonly function is either a) readnone or b) UB. This means we can't actually infer much unless nocapture has already been inferred.

[reames]

Has the same propagation bug fixed in 7b54de5f, need to rework.

[reames]

Rework account for only being to infer on nocapture arguments.

[nikic]

LGTM

llvm/lib/Transforms/IPO/FunctionAttrs.cpp

728

This could be written as CB.doesNotReadMemory() || CB.doesNotReadMemory(UseIndex) for symmetry with the other cases. Though personally I find the naming of these methods really unfortunate (they should probably be called onlyWritesMemory()) so maybe the explicit form is better.

[reames]

llvm/lib/Transforms/IPO/FunctionAttrs.cpp

728

I'm leaving this as is, but I agree we could use some naming cleanup here. (I may even do it since this is the second time I've hit this.)

Though to highlight "does not read memory" and "WriteOnly" are not quite the same. The former allows readnone which is correct, but surprising here. Personally, I hate subtle semantic surprises and am increasing of the view that we should check the attributes we mean. (At least in inference code.)

clang/test/CodeGen/arm-vfp16-arguments.c

 
 void test_hfa(hfa_t a) {}
 // CHECK-SOFT: define{{.*}} void @test_hfa([2 x i64] %a.coerce)
 // CHECK-HARD: define{{.*}} arm_aapcs_vfpcc void @test_hfa([2 x <2 x i32>] %a.coerce)
 // CHECK-FULL: define{{.*}} arm_aapcs_vfpcc void @test_hfa(%struct.hfa_t %a.coerce)
 
 hfa_t ghfa;
 hfa_t test_ret_hfa(void) { return ghfa; }
-// CHECK-SOFT: define{{.*}} void @test_ret_hfa(%struct.hfa_t* noalias nocapture sret(%struct.hfa_t) align 8 %agg.result)
+// CHECK-SOFT: define{{.*}} void @test_ret_hfa(%struct.hfa_t* noalias nocapture writeonly sret(%struct.hfa_t) align 8 %agg.result)
 // CHECK-HARD: define{{.*}} arm_aapcs_vfpcc [2 x <2 x i32>] @test_ret_hfa()
 // CHECK-FULL: define{{.*}} arm_aapcs_vfpcc %struct.hfa_t @test_ret_hfa()

clang/test/CodeGenCXX/wasm-args-returns.cpp

 //
 // CHECK: declare void @_Z3use9one_field(double)
 // CHECK: declare double @_Z13def_one_fieldv()
 
 struct two_fields {
   double d, e;
 };
 test(two_fields);
-// CHECK: define void @_Z7forward10two_fields(%struct.two_fields* noalias nocapture sret(%struct.two_fields) align 8 %{{.*}}, %struct.two_fields* nocapture readonly byval(%struct.two_fields) align 8 %{{.*}})
+// CHECK: define void @_Z7forward10two_fields(%struct.two_fields* noalias nocapture writeonly sret(%struct.two_fields) align 8 %{{.*}}, %struct.two_fields* nocapture readonly byval(%struct.two_fields) align 8 %{{.*}})
 //
 // CHECK: define void @_Z15test_two_fieldsv()
 // CHECK: %[[tmp:.*]] = alloca %struct.two_fields, align 8
 // CHECK: call void @_Z14def_two_fieldsv(%struct.two_fields* nonnull sret(%struct.two_fields) align 8 %[[tmp]])
 // CHECK: call void @_Z3use10two_fields(%struct.two_fields* nonnull byval(%struct.two_fields) align 8 %[[tmp]])
 // CHECK: ret void
 //
 // CHECK: declare void @_Z3use10two_fields(%struct.two_fields* byval(%struct.two_fields) align 8)

clang/test/CodeGenOpenCL/amdgpu-abi-struct-coerce.cl

 // CHECK: define{{.*}} %struct.struct_arr16 @func_ret_struct_arr16()
 // CHECK: ret %struct.struct_arr16 zeroinitializer
 struct_arr16 func_ret_struct_arr16()
 {
   struct_arr16 s = { 0 };
   return s;
 }
 
-// CHECK: define{{.*}} void @func_ret_struct_arr32(%struct.struct_arr32 addrspace(5)* noalias nocapture sret(%struct.struct_arr32) align 4 %agg.result)
+// CHECK: define{{.*}} void @func_ret_struct_arr32(%struct.struct_arr32 addrspace(5)* noalias nocapture writeonly sret(%struct.struct_arr32) align 4 %agg.result)
 struct_arr32 func_ret_struct_arr32()
 {
   struct_arr32 s = { 0 };
   return s;
 }
 
-// CHECK: define{{.*}} void @func_ret_struct_arr33(%struct.struct_arr33 addrspace(5)* noalias nocapture sret(%struct.struct_arr33) align 4 %agg.result)
+// CHECK: define{{.*}} void @func_ret_struct_arr33(%struct.struct_arr33 addrspace(5)* noalias nocapture writeonly sret(%struct.struct_arr33) align 4 %agg.result)
 struct_arr33 func_ret_struct_arr33()
 {
   struct_arr33 s = { 0 };
   return s;
 }
 
 // CHECK: define{{.*}} %struct.struct_char_arr32 @func_ret_struct_char_arr32()
 struct_char_arr32 func_ret_struct_char_arr32()

 double_nested_struct func_double_nested_struct_ret(int4 arg0, int arg1) {
   double_nested_struct s = { 0 };
   return s;
 }
 
 // CHECK: define{{.*}} void @func_large_struct_padding_arg_direct(i8 %arg.coerce0, i32 %arg.coerce1, i8 %arg.coerce2, i32 %arg.coerce3, i8 %arg.coerce4, i8 %arg.coerce5, i16 %arg.coerce6, i16 %arg.coerce7, [3 x i8] %arg.coerce8, i64 %arg.coerce9, i32 %arg.coerce10, i8 %arg.coerce11, i32 %arg.coerce12, i16 %arg.coerce13, i8 %arg.coerce14)
 void func_large_struct_padding_arg_direct(large_struct_padding arg) { }
 
-// CHECK: define{{.*}} void @func_large_struct_padding_arg_store(%struct.large_struct_padding addrspace(1)* nocapture %out, %struct.large_struct_padding addrspace(5)* nocapture readonly byval(%struct.large_struct_padding) align 8 %arg)
+// CHECK: define{{.*}} void @func_large_struct_padding_arg_store(%struct.large_struct_padding addrspace(1)* nocapture writeonly %out, %struct.large_struct_padding addrspace(5)* nocapture readonly byval(%struct.large_struct_padding) align 8 %arg)
 void func_large_struct_padding_arg_store(global large_struct_padding* out, large_struct_padding arg) {
   *out = arg;
 }
 
 // CHECK: define{{.*}} void @v3i32_reg_count(<3 x i32> %arg1, <3 x i32> %arg2, <3 x i32> %arg3, <3 x i32> %arg4, i32 %arg5.coerce0, float %arg5.coerce1, i32 %arg5.coerce2)
 void v3i32_reg_count(int3 arg1, int3 arg2, int3 arg3, int3 arg4, struct_arg_t arg5) { }
 
 // Function signature from blender, nothing should be passed byval. The v3i32

llvm/lib/Transforms/IPO/FunctionAttrs.cpp

case Instruction::Invoke: {
             SCCNodes.count(F->getArg(UseIndex)))
           // This is an argument which is part of the speculative SCC.  Note
           // that only operands corresponding to formal arguments of the callee
           // can participate in the speculation.
           break;
 
       // The accessors used on call site here do the right thing for calls and
       // invokes with operand bundles.
-      if (!CB.onlyReadsMemory() && !CB.onlyReadsMemory(UseIndex))
-        return Attribute::None;
-      if (!CB.doesNotAccessMemory(UseIndex))
+      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;
+      }
       break;
     }
 
     case Instruction::Load:
       // A volatile load has side effects beyond what readonly can be relied
       // upon.
       if (cast<LoadInst>(I)->isVolatile())
         return Attribute::None;

llvm/test/Analysis/TypeBasedAliasAnalysis/functionattrs.ll

 ; isn't necessarily invalid.
 
 ; CHECK: define void @test2_yes(i8* nocapture %p, i8* nocapture %q, i64 %n) #4 {
 define void @test2_yes(i8* %p, i8* %q, i64 %n) nounwind {
   call void @llvm.memcpy.p0i8.p0i8.i64(i8* %p, i8* %q, i64 %n, i1 false), !tbaa !1
   ret void
 }
 
-; CHECK: define void @test2_no(i8* nocapture %p, i8* nocapture readonly %q, i64 %n) #5 {
+; CHECK: define void @test2_no(i8* nocapture writeonly %p, i8* nocapture readonly %q, i64 %n) #5 {
 define void @test2_no(i8* %p, i8* %q, i64 %n) nounwind {
   call void @llvm.memcpy.p0i8.p0i8.i64(i8* %p, i8* %q, i64 %n, i1 false), !tbaa !2
   ret void
 }
 
 ; Similar to the others, va_arg only accesses memory through its operand.
 
 ; CHECK: define i32 @test3_yes(i8* nocapture %p) #0 {

llvm/test/Other/cgscc-devirt-iteration.ll

 ; creates the memcpy intrinsic call, and we rely on the count of indirect calls
 ; decreasing and the count of direct calls increasing.
 ; Adding 'noinline' attribute to force attributes for improved matching.
 define void @test3(i8* %src, i8* %dest, i64 %size) noinline {
 ; CHECK: Function Attrs
 ; CHECK-NOT: read
 ; CHECK-SAME: noinline
 ; BEFORE-LABEL: define void @test3(i8* %src, i8* %dest, i64 %size)
-; AFTER-LABEL: define void @test3(i8* nocapture readonly %src, i8* nocapture %dest, i64 %size)
+; AFTER-LABEL: define void @test3(i8* nocapture readonly %src, i8* nocapture writeonly %dest, i64 %size)
   %fptr = alloca i8* (i8*, i8*, i64)*
   store i8* (i8*, i8*, i64)* @memcpy, i8* (i8*, i8*, i64)** %fptr
   %f = load i8* (i8*, i8*, i64)*, i8* (i8*, i8*, i64)** %fptr
   call i8* %f(i8* %dest, i8* %src, i64 %size)
 ; CHECK: call void @llvm.memcpy
   ret void
 }
 

llvm/test/Transforms/FunctionAttrs/norecurse.ll

 
 ; CHECK: Function Attrs
 ; CHECK-NEXT: declare i32 @k()
 declare i32 @k() readnone
 
 ; CHECK: Function Attrs
 ; CHECK-SAME: nounwind
 ; CHECK-NOT: norecurse
-; CHECK-NEXT: define void @intrinsic(i8* nocapture %dest, i8* nocapture readonly %src, i32 %len)
+; CHECK-NEXT: define void @intrinsic(i8* nocapture writeonly %dest, i8* nocapture readonly %src, i32 %len)
 define void @intrinsic(i8* %dest, i8* %src, i32 %len) {
   call void @llvm.memcpy.p0i8.p0i8.i32(i8* %dest, i8* %src, i32 %len, i1 false)
   ret void
 }
 
 ; CHECK: Function Attrs
 ; CHECK-NEXT: declare void @llvm.memcpy.p0i8.p0i8.i32
 declare void @llvm.memcpy.p0i8.p0i8.i32(i8*, i8*, i32, i1)

llvm/test/Transforms/FunctionAttrs/writeonly.ll

 ; CHECK: define void @direct1(i8* %p)
 define void @direct1(i8* %p) {
   call void @direct1_callee(i8* %p)
   ret void
 }
 
 declare void @direct2_callee(i8* %p) writeonly
 
+; writeonly w/o nocapture is not enough
 ; CHECK: define void @direct2(i8* %p)
 define void @direct2(i8* %p) {
   call void @direct2_callee(i8* %p)
+  ; read back from global, read through pointer...
   ret void
 }
 
-declare void @direct3_callee(i8* writeonly %p)
+; CHECK: define void @direct2b(i8* nocapture writeonly %p)
+define void @direct2b(i8* %p) {
+  call void @direct2_callee(i8* nocapture %p)
+  ret void
+}
+
+declare void @direct3_callee(i8* nocapture writeonly %p)
 
-; CHECK: define void @direct3(i8* %p)
+; CHECK: define void @direct3(i8* nocapture 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 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 readonly %f)
+; CHECK: define void @fptr_test2(i8* nocapture writeonly %p, void (i8*)* nocapture readonly %f)
 define void @fptr_test2(i8* %p, void (i8*)* %f) {
-  call void %f(i8* writeonly %p)
+  call void %f(i8* nocapture writeonly %p)
   ret void
 }
 
-; CHECK: define void @fptr_test3(i8* %p, void (i8*)* nocapture readonly %f)
+; CHECK: define void @fptr_test3(i8* nocapture writeonly %p, void (i8*)* nocapture readonly %f)
 define void @fptr_test3(i8* %p, void (i8*)* %f) {
-  call void %f(i8* %p) writeonly
+  call void %f(i8* nocapture %p) writeonly
   ret void
 }
 
 ; CHECK: attributes #0 = { {{.*}}readnone{{.*}} }
 ; CHECK: attributes #1 = { {{.*}}readonly{{.*}} }
 ; CHECK: attributes #2 = { {{.*}}writeonly{{.*}} }