[IR] ArgMemOnly functions with WriteOnly ptr args do not read memory.

[fhahn]

Summary

Calls/functions with ArgMemOnly only access memory via pointer-type
arguments. If all pointer-type arguments are WriteOnly, this means that
the function does not read memory.

This is relevant, for example, for the current definition of
@llvm.memset, which is marked as ArgMemOnly with WriteOnly pointers arguments.
Without this patch, doesNotReadMemory returns false for @llvm.memset.

I am not sure where the best place to add tests for this would be?

This impacts a single attributor test, which uses the following
declaration

declare <4 x i32> @test11_1(<4 x i32*>) argmemonly nounwind readonly

With the patch, this function will be treated as readnone. I think that
is fine according to the ArgMemOnly definition: there are no
pointer-typed arguments (the only argument is a vector of pointers), so
no memory can be accessed.

If 'pointer-typed arguments' could be interpreted as any argument containing a
pointer, this patch would have to be more careful with checking the arguments.

[efriedma]

I'd prefer to just add writeonly markings in places where they're missing. For llvm.memset in particular, can we mark it IntrWriteMem?

[merge_guards_bot]

Unit tests: pass. 61772 tests passed, 0 failed and 780 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[jdoerfert]

In D72666#1818407, @efriedma wrote:

I'd prefer to just add writeonly markings in places where they're missing. For llvm.memset in particular, can we mark it IntrWriteMem?

We could "deduce" it, based on this logic, and then add writeonly. That would work for non intrinsics as well.

[fhahn]

In D72666#1818532, @jdoerfert wrote:

In D72666#1818407, @efriedma wrote:

I'd prefer to just add writeonly markings in places where they're missing. For llvm.memset in particular, can we mark it IntrWriteMem?

Yes we should probably just mark llvm.memset as IntrWriteMem.

We could "deduce" it, based on this logic, and then add writeonly. That would work for non intrinsics as well.

Yep I think it might be beneficial to use the logic in the patch to set the write-only function attribute at the time attributes are added to a call/function. I am not sure how common this is (argmemonly + writeOnly/readOnly argument attributes), but that it was missing for an intrinsic like memset seems to indicate how easy things like that can be missed. Alternatively we could use the logic to warn/fail on missing attributes. WDYT?

[jdoerfert]

In D72666#1820862, @fhahn wrote:

In D72666#1818532, @jdoerfert wrote:

In D72666#1818407, @efriedma wrote:

I'd prefer to just add writeonly markings in places where they're missing. For llvm.memset in particular, can we mark it IntrWriteMem?

Yes we should probably just mark llvm.memset as IntrWriteMem.

Agreed.

We could "deduce" it, based on this logic, and then add writeonly. That would work for non intrinsics as well.

Yep I think it might be beneficial to use the logic in the patch to set the write-only function attribute at the time attributes are added to a call/function. I am not sure how common this is (argmemonly + writeOnly/readOnly argument attributes), but that it was missing for an intrinsic like memset seems to indicate how easy things like that can be missed. Alternatively we could use the logic to warn/fail on missing attributes. WDYT?

I dislike the warn/fail idea and I would still argue for deduction instead:

1. We will hopefully soon have a way for users to add attributes like argmemonly and writeonly, so if they miss one we can just infer it.
2. It is also a "shortcut" if we can infer it if after partial code specialization eliminated the read arguments or the non-argument accesses.

[fhahn]

In D72666#1820906, @jdoerfert wrote:

In D72666#1820862, @fhahn wrote:

In D72666#1818532, @jdoerfert wrote:

In D72666#1818407, @efriedma wrote:

I'd prefer to just add writeonly markings in places where they're missing. For llvm.memset in particular, can we mark it IntrWriteMem?

Yes we should probably just mark llvm.memset as IntrWriteMem.

Agreed.

Posted D72789.

We could "deduce" it, based on this logic, and then add writeonly. That would work for non intrinsics as well.

Yep I think it might be beneficial to use the logic in the patch to set the write-only function attribute at the time attributes are added to a call/function. I am not sure how common this is (argmemonly + writeOnly/readOnly argument attributes), but that it was missing for an intrinsic like memset seems to indicate how easy things like that can be missed. Alternatively we could use the logic to warn/fail on missing attributes. WDYT?

I dislike the warn/fail idea and I would still argue for deduction instead:

1. We will hopefully soon have a way for users to add attributes like argmemonly and writeonly, so if they miss one we can just infer it.
2. It is also a "shortcut" if we can infer it if after partial code specialization eliminated the read arguments or the non-argument accesses.

I'll check how to best do it at call/declare construction time.

[reames]

This should definitely be an inference rule. See FunctionAttrs.

[lebedev.ri]

This review seems to be stuck/dead, consider abandoning if no longer relevant.

[lebedev.ri]

This review seems to be stuck/dead, consider abandoning if no longer relevant.

llvm/include/llvm/IR/Function.h

bool onlyReadsMemory() const {
     return doesNotAccessMemory() || hasFnAttribute(Attribute::ReadOnly);
   }
   void setOnlyReadsMemory() {
     addFnAttr(Attribute::ReadOnly);
   }
 
   /// Determine if the function does not access or only writes memory.
   bool doesNotReadMemory() const {
-    return doesNotAccessMemory() || hasFnAttribute(Attribute::WriteOnly);
+    return doesNotAccessMemory() || hasFnAttribute(Attribute::WriteOnly) ||
+           (hasFnAttribute(Attribute::ArgMemOnly) &&
+            all_of(args(), [](const Argument &A) {
+              return !A.getType()->isPointerTy() ||
+                     A.hasAttribute(Attribute::WriteOnly);
+            }));
   }
   void setDoesNotReadMemory() {
     addFnAttr(Attribute::WriteOnly);
   }
 
   /// Determine if the call can access memmory only using pointers based
   /// on its arguments.
   bool onlyAccessesArgMemory() const {

llvm/include/llvm/IR/InstrTypes.h

bool onlyReadsMemory() const {
     return doesNotAccessMemory() || hasFnAttr(Attribute::ReadOnly);
   }
   void setOnlyReadsMemory() {
     addAttribute(AttributeList::FunctionIndex, Attribute::ReadOnly);
   }
 
   /// Determine if the call does not access or only writes memory.
   bool doesNotReadMemory() const {
-    return doesNotAccessMemory() || hasFnAttr(Attribute::WriteOnly);
+    return doesNotAccessMemory() || hasFnAttr(Attribute::WriteOnly) ||
+           (onlyAccessesArgMemory() && argsWriteOnly());
   }
   void setDoesNotReadMemory() {
     addAttribute(AttributeList::FunctionIndex, Attribute::WriteOnly);
   }
 
   /// Determine if the call can access memmory only using pointers based
   /// on its arguments.
   bool onlyAccessesArgMemory() const {
template <typename AttrKind> bool hasFnAttrImpl(AttrKind Kind) const {
 
     // Operand bundles override attributes on the called function, but don't
     // override attributes directly present on the call instruction.
     if (isFnAttrDisallowedByOpBundle(Kind))
       return false;
 
     return hasFnAttrOnCalledFunction(Kind);
   }
+
+  bool argsWriteOnly() const {
+    for (unsigned I = 0; I < getNumArgOperands(); I++)
+      if (getArgOperand(I)->getType()->isPointerTy() &&
+          !paramHasAttr(I, Attribute::WriteOnly))
+        return false;
+    return true;
+  }
 };
 
 template <>
 struct OperandTraits<CallBase> : public VariadicOperandTraits<CallBase, 1> {};
 
 DEFINE_TRANSPARENT_OPERAND_ACCESSORS(CallBase, Value)
 
 //===----------------------------------------------------------------------===//

llvm/test/Transforms/Attributor/readattrs.ll

 ; ATTRIBUTOR: define <4 x i32> @test10
 define <4 x i32> @test10(<4 x i32*> %ptrs) {
   %res = call <4 x i32> @llvm.masked.gather.v4i32.v4p0i32(<4 x i32*> %ptrs, i32 4, <4 x i1><i1 true, i1 false, i1 true, i1 false>, <4 x i32>undef)
   ret <4 x i32> %res
 }
 
 ; ATTRIBUTOR: declare <4 x i32> @test11_1
 declare <4 x i32> @test11_1(<4 x i32*>) argmemonly nounwind readonly
-; ATTRIBUTOR: readonly
-; ATTRIBUTOR-NOT: readnone
+; ATTRIBUTOR: readnone
 ; ATTRIBUTOR: define <4 x i32> @test11_2
 define <4 x i32> @test11_2(<4 x i32*> %ptrs) {
   %res = call <4 x i32> @test11_1(<4 x i32*> %ptrs)
   ret <4 x i32> %res
 }
 
 declare <4 x i32> @test12_1(<4 x i32*>) argmemonly nounwind
 ; ATTRIBUTOR-NOT: readnone