A readonly operand bundle should not prevent inference of readonly from a readnone callee

[reames]

Summary

A readonly operand bundle disallows inference of readnone from the callee, but it should not prevent us from using the readnone fact on the callee to infer readonly for the callsite.

Fixes pr53270.

Review note: The change for writeonly is an unrelated bug fix, and I'll be landing it separately and rebasing. Included it here just because the cycle time on building changes to such a key header are painful.

[fhahn]

LGTM, thanks!

llvm/include/llvm/IR/InstrTypes.h

2291

nit: doc-comment should use ///?

2293

Nit: 2 spaces before This.

2294

nit: 2 spaces before The.

llvm/test/Transforms/InstCombine/trivial-dse-calls.ll

258

Do we also have a test with an unknown bundle?

[reames]

llvm/include/llvm/IR/InstrTypes.h

2293

There appear to already be two? Did you maybe mean you wanted one? (At least from my memory of high school grammar, two is "correct".)

llvm/test/Transforms/InstCombine/trivial-dse-calls.ll

258

Should be covered by test/Features/OperandBundles/func-attrs.ll

[fhahn]

llvm/include/llvm/IR/InstrTypes.h

2293

Oh right, I can only speak from experience in other places in LLVM and it seems more often than not single spaces are used. Not a big deal though, I just thought it might have been a typo.

[reames]

llvm/include/llvm/IR/InstrTypes.h

2293

I think this is one of those where actual usage varies widely, and it doesn't really matter.

llvm/include/llvm/IR/InstrTypes.h

public:
   bool isNoInline() const { return hasFnAttr(Attribute::NoInline); }
   void setIsNoInline() { addFnAttr(Attribute::NoInline); }
   /// Determine if the call does not access memory.
   bool doesNotAccessMemory() const { return hasFnAttr(Attribute::ReadNone); }
   void setDoesNotAccessMemory() { addFnAttr(Attribute::ReadNone); }
 
   /// Determine if the call does not access or only reads memory.
   bool onlyReadsMemory() const {
-    return doesNotAccessMemory() || hasFnAttr(Attribute::ReadOnly);
+    return hasImpliedFnAttr(Attribute::ReadOnly);
   }
 
   void setOnlyReadsMemory() { addFnAttr(Attribute::ReadOnly); }
 
   /// Determine if the call does not access or only writes memory.
   bool onlyWritesMemory() const {
-    return doesNotAccessMemory() || hasFnAttr(Attribute::WriteOnly);
+    return hasImpliedFnAttr(Attribute::WriteOnly);
   }
   void setOnlyWritesMemory() { addFnAttr(Attribute::WriteOnly); }
 
   /// Determine if the call can access memmory only using pointers based
   /// on its arguments.
   bool onlyAccessesArgMemory() const {
     return hasFnAttr(Attribute::ArgMemOnly);
   }
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);
   }
 
+  /// A specialized version of hasFnAttrImpl for when the caller wants to
+  /// know if an attribute's semantics are implied, not whether the attribute
+  /// is actually present.  This distinction only exists when checking whether
+  /// something is readonly or writeonly since readnone implies both.  The case
+  /// which motivates the specialized code is a callee with readnone, and an
+  /// operand bundle on the call which disallows readnone but not either
+  /// readonly or writeonly.
+  bool hasImpliedFnAttr(Attribute::AttrKind Kind) const {
+    assert((Kind == Attribute::ReadOnly || Kind == Attribute::WriteOnly) &&
+           "use hasFnAttrImpl instead");
+    if (Attrs.hasFnAttr(Kind) || Attrs.hasFnAttr(Attribute::ReadNone))
+      return true;
+
+    if (isFnAttrDisallowedByOpBundle(Kind))
+      return false;
+
+    return hasFnAttrOnCalledFunction(Kind) ||
+      hasFnAttrOnCalledFunction(Attribute::ReadNone);
+  }
+
   /// Determine whether the return value has the given attribute. Supports
   /// Attribute::AttrKind and StringRef as \p AttrKind types.
   template <typename AttrKind> bool hasRetAttrImpl(AttrKind Kind) const {
     if (Attrs.hasRetAttr(Kind))
       return true;
 
     // Look at the callee, if available.
     if (const Function *F = getCalledFunction())

llvm/test/Transforms/InstCombine/trivial-dse-calls.ll

 ; CHECK-NEXT:    ret void
 ;
   call void @removable_readnone()
   ret void
 }
 
 define void @test_readnone_with_deopt() {
 ; CHECK-LABEL: @test_readnone_with_deopt(
-; CHECK-NEXT:    call void @removable_readnone() [ "deopt"() ]
 ; CHECK-NEXT:    ret void
 ;
   call void @removable_readnone() [ "deopt"() ]
   ret void
 }
 
 define void @test_readonly() {
 ; CHECK-LABEL: @test_readonly(
 ; CHECK-NEXT:    ret void
 ;
   call void @removable_ro()