LLVM: Optimization Pass: Remove conflicting attribute, if any, before adding new read attribute to an argument

[anhtuyen]

Summary

Update optimization pass to prevent adding read-attribute to an argument without removing its conflicting attribute.

A read attribute, based on the result of the attribute deduction process, might be added to an argument. The attribute might be in conflict with other read/write attribute currently associated with the argument. To ensure the compatibility of attributes, conflicting attribute, if any, must be removed before a new one is added.

The following snippet shows the current behavior of the compiler, where the compilation process is aborted due to incompatible attributes.

$ cat x.ll
; ModuleID = 'x.bc'


%_type_of_d-ccc = type <{ i8*, i8, i8, i8, i8 }>

@d-ccc = internal global %_type_of_d-ccc <{ i8* null, i8 1, i8 13, i8 0, i8 -127 }>, align 8

define void @foo(i32* writeonly %.aaa) {
foo_entry:
  %_param_.aaa = alloca i32*, align 8
  store i32* %.aaa, i32** %_param_.aaa, align 8
  store i8 0, i8* getelementptr inbounds (%_type_of_d-ccc, %_type_of_d-ccc* @d-ccc, i32 0, i32 3)
  ret void
}

$ opt -O3 x.ll
Attributes 'readnone and writeonly' are incompatible!
void (i32*)* @foo
in function foo
LLVM ERROR: Broken function found, compilation aborted!

The purpose of this changeset is to fix the above error. This fix is based on a suggestion from Johannes @jdoerfert (many thanks!!!)

[rnk]

This seems like the wrong fix, I would expect functionattrs to improve the deduction by removing the writeonly attribute.

Despite its name, the readnone attribute has implied semantics about write operations:

readnone makes more sense when you consider it as a strengthening of readonly, which implies no write operations.

llvm/test/Transforms/FunctionAttrs/writeonly.ll

1–3 ↗

(On Diff #188436)

Please just run functionattrs directly.

[anhtuyen]

In D58694#1411172, @rnk wrote:

This seems like the wrong fix, I would expect functionattrs to improve the deduction by removing the writeonly attribute.

Despite its name, the readnone attribute has implied semantics about write operations:

readnone makes more sense when you consider it as a strengthening of readonly, which implies no write operations.

Removing an attribute explicitly added by the user (writeonly) and replacing it with the attribute we deduced (readnone) ? That might not be the improvement we want.
A side note, but the snippet was reduced from an actual code, where the writeonly is required.

[rnk]

In D58694#1411211, @anhtuyen wrote:

Removing an attribute explicitly added by the user (writeonly) and replacing it with the attribute we deduced (readnone) ? That might not be the improvement we want.
A side note, but the snippet was reduced from an actual code, where the writeonly is required.

readnone on the entire function implies the input parameter isn't read, so the inferred attribute implies what was written by the user. Users write many things, but the purpose of optimization is often to replace them with better things, and I don't see how this is much different.

[chandlerc]

In D58694#1411343, @rnk wrote:

In D58694#1411211, @anhtuyen wrote:

Removing an attribute explicitly added by the user (writeonly) and replacing it with the attribute we deduced (readnone) ? That might not be the improvement we want.
A side note, but the snippet was reduced from an actual code, where the writeonly is required.

readnone on the entire function implies the input parameter isn't read, so the inferred attribute implies what was written by the user. Users write many things, but the purpose of optimization is often to replace them with better things, and I don't see how this is much different.

+1

Despite the confusing name, both readonly and writeonly can be strengthened to readnone.

[anhtuyen]

Thank you very much, @rnk and @chandlerc for your comments. Let me change the fix accordingly.

[anhtuyen]

Based on comments from the reviewers, if a pointer argument with attribute WriteOnly has no uses, there is no write through nor read from the pointer argument. In this case, its attribute can safely be strengthened from WriteOnly to ReadNone

[jdoerfert]

llvm/lib/Transforms/IPO/FunctionAttrs.cpp

281 ↗

(On Diff #193542)

In my opinion, this is what you want to do when a new attribute is derived: Clear the existing incompatible ones.

730 ↗

(On Diff #193542)

In general, I dislike specialization for write-only in the first place. If that is voted in, we should make at least the reasoning more explicit: Initializing R with readnon above is hard to follow. It should be placed inside this conditional instead.

[jdoerfert]

llvm/lib/Transforms/IPO/FunctionAttrs.cpp

281 ↗

(On Diff #193542)

To make it clearer, this is what we *always* should do when a new attribute is derived, not only for *write-only* attributes at this particular position you modified.

[anhtuyen]

llvm/lib/Transforms/IPO/FunctionAttrs.cpp

730 ↗

(On Diff #193542)

Hi @jdoerfert , will you like it better if I move the code to handle writeonly (and additionally readonly) to inside the function determinePointerReadAttrs ?

[jdoerfert]

llvm/lib/Transforms/IPO/FunctionAttrs.cpp

730 ↗

(On Diff #193542)

I prefer replacing all the code, see D59918 and D59980.

For now, I'd argue wherever we have something like
A->addAttr(R);
we should remove conflicting arguments.

So make a function, say addReadAttr(AttrKind R),
which (1) removes conflicting attributes, (2) adds the new attribute, and (3) does the bookkeping (statistics).

[anhtuyen]

My apologies about the long pause before providing this patch. I was on a long vacation, and was then driven into some other issues until now. This new patch will remove conflicting attribute, if any, currently associated with an argument before adding a read attribute to it. It also updates the statistics accordingly.

[jdoerfert]

llvm/lib/Transforms/IPO/FunctionAttrs.cpp

684 ↗

(On Diff #217692)

Why do you decrement the statistics here?

685 ↗

(On Diff #217692)

There is no need to check if an attribute is present before removing it.

[anhtuyen]

llvm/lib/Transforms/IPO/FunctionAttrs.cpp

684 ↗

(On Diff #217692)

I thought we increment the count when we added it, so removing it would require a decrement. Is it not right ?

685 ↗

(On Diff #217692)

Although we can remove the attribute even when the argument does not have it, the check is to make sure we decrement the corresponding counter.

[jdoerfert]

llvm/lib/Transforms/IPO/FunctionAttrs.cpp

684 ↗

(On Diff #217692)

We usually don't do that (here). I would not recommend it anyway.

685 ↗

(On Diff #217692)

See above.

[anhtuyen]

Address @jdoerfert Johannes's comment about not decrementing the statistics.

[anhtuyen]

Hello,
By the end of last week, I have addressed all comments from the reviewers. Please let me know if there is any other issue, which to like me to handle so that we can complete the patch. Thank you very much!

[rnk]

lgtm

llvm/trunk/lib/Transforms/IPO/FunctionAttrs.cpp

for (Instruction &I : Entry) {
     }
     if (!isGuaranteedToTransferExecutionToSuccessor(&I))
       break;
   }
 
   return Changed;
 }
 
+static bool addReadAttr(Argument *A, Attribute::AttrKind R) {
+  assert((R == Attribute::ReadOnly || R == Attribute::ReadNone)
+         && "Must be a Read attribute.");
+  assert(A && "Argument must not be null.");
+
+  // If the argument already has the attribute, nothing needs to be done.
+  if (A->hasAttribute(R))
+      return false;
+
+  // Otherwise, remove potentially conflicting attribute, add the new one,
+  // and update statistics.
+  A->removeAttr(Attribute::WriteOnly);
+  A->removeAttr(Attribute::ReadOnly);
+  A->removeAttr(Attribute::ReadNone);
+  A->addAttr(R);
+  R == Attribute::ReadOnly ? ++NumReadOnlyArg : ++NumReadNoneArg;
+  return true;
+}
+
 /// Deduce nocapture attributes for the SCC.
 static bool addArgumentAttrs(const SCCNodeSet &SCCNodes) {
   bool Changed = false;
 
   ArgumentGraph AG;
 
   // Check each function in turn, determining which pointer arguments are not
   // captured.
for (Function::arg_iterator A = F->arg_begin(), E = F->arg_end(); A != E;
       if (!HasNonLocalUses && !A->onlyReadsMemory()) {
         // Can we determine that it's readonly/readnone without doing an SCC?
         // Note that we don't allow any calls at all here, or else our result
         // will be dependent on the iteration order through the functions in the
         // SCC.
         SmallPtrSet<Argument *, 8> Self;
         Self.insert(&*A);
         Attribute::AttrKind R = determinePointerReadAttrs(&*A, Self);
-        if (R != Attribute::None) {
-          A->addAttr(R);
-          Changed = true;
-          R == Attribute::ReadOnly ? ++NumReadOnlyArg : ++NumReadNoneArg;
-        }
+        if (R != Attribute::None)
+          Changed = addReadAttr(A, R);
       }
     }
   }
 
   // The graph we've collected is partial because we stopped scanning for
   // argument uses once we solved the argument trivially. These partial nodes
   // show up as ArgumentGraphNode objects with an empty Uses list, and for
   // these nodes the final decision about whether they capture has already been
for (unsigned i = 0, e = ArgumentSCC.size(); i != e; ++i) {
       }
       ReadAttr = K;
       break;
     }
 
     if (ReadAttr != Attribute::None) {
       for (unsigned i = 0, e = ArgumentSCC.size(); i != e; ++i) {
         Argument *A = ArgumentSCC[i]->Definition;
-        // Clear out existing readonly/readnone attributes
-        A->removeAttr(Attribute::ReadOnly);
-        A->removeAttr(Attribute::ReadNone);
-        A->addAttr(ReadAttr);
-        ReadAttr == Attribute::ReadOnly ? ++NumReadOnlyArg : ++NumReadNoneArg;
-        Changed = true;
+        Changed = addReadAttr(A, ReadAttr);
       }
     }
   }
 
   return Changed;
 }
 
 /// Tests whether a function is "malloc-like".

llvm/trunk/test/Transforms/FunctionAttrs/writeonly.ll

+; RUN: opt < %s -functionattrs         -S | FileCheck %s
+; RUN: opt < %s -passes=function-attrs -S | FileCheck %s
+
+; CHECK: define void @nouses-argworn-funrn(i32* nocapture readnone %.aaa) #0 {
+define void @nouses-argworn-funrn(i32* writeonly %.aaa) {
+nouses-argworn-funrn_entry:
+  ret void
+}
+
+; CHECK: define void @nouses-argworn-funro(i32* nocapture readnone %.aaa, i32* nocapture readonly %.bbb) #1 {
+define void @nouses-argworn-funro(i32* writeonly %.aaa, i32* %.bbb) {
+nouses-argworn-funro_entry:
+  %val = load i32 , i32* %.bbb
+  ret void
+}
+
+%_type_of_d-ccc = type <{ i8*, i8, i8, i8, i8 }>
+
+@d-ccc = internal global %_type_of_d-ccc <{ i8* null, i8 1, i8 13, i8 0, i8 -127 }>, align 8
+
+; CHECK: define void @nouses-argworn-funwo(i32* nocapture readnone %.aaa) #2 {
+define void @nouses-argworn-funwo(i32* writeonly %.aaa) {
+nouses-argworn-funwo_entry:
+  store i8 0, i8* getelementptr inbounds (%_type_of_d-ccc, %_type_of_d-ccc* @d-ccc, i32 0, i32 3)
+  ret void
+}
+
+; CHECK: attributes #0 = { {{.*}} readnone }
+; CHECK: attributes #1 = { {{.*}} readonly }
+; CHECK: attributes #2 = { {{.*}} writeonly }