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

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

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

736

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

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

736

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

736

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

679

Why do you decrement the statistics here?

680

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

[anhtuyen]

llvm/lib/Transforms/IPO/FunctionAttrs.cpp

679

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

680

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

679

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

680

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/lib/Transforms/IPO/FunctionAttrs.cpp

for (Function *F : SCCNodes) {
     if (F->doesNotReadMemory() && WritesMemory)
       continue;
 
     MadeChange = true;
 
     // Clear out any existing attributes.
     F->removeFnAttr(Attribute::ReadOnly);
     F->removeFnAttr(Attribute::ReadNone);
     F->removeFnAttr(Attribute::WriteOnly);
 
     if (!WritesMemory && !ReadsMemory) {
       // Clear out any "access range attributes" if readnone was deduced.
       F->removeFnAttr(Attribute::ArgMemOnly);
       F->removeFnAttr(Attribute::InaccessibleMemOnly);
       F->removeFnAttr(Attribute::InaccessibleMemOrArgMemOnly);
     }
 
determinePointerReadAttrs(Argument *A,
                           const SmallPtrSet<Argument *, 8> &SCCNodes) {
   SmallVector<Use *, 32> Worklist;
   SmallPtrSet<Use *, 32> Visited;
 
   // inalloca arguments are always clobbered by the call.
   if (A->hasInAllocaAttr())
     return Attribute::None;
 
+  // If the argument is WriteOnly, no read-attribute should be added.
+  if (A->hasAttribute(Attribute::WriteOnly))
+    return Attribute::None;
+
   bool IsRead = false;
   // We don't need to track IsWritten. If A is written to, return immediately.
 
   for (Use &U : A->uses()) {
     Visited.insert(&U);
     Worklist.push_back(&U);
   }
 
for (Function *F : SCCNodes) {
     // We can infer and propagate function attributes only when we know that the
     // definition we'll get at link time is *exactly* the definition we see now.
     // For more details, see GlobalValue::mayBeDerefined.
     if (!F->hasExactDefinition())
       continue;
 
     Changed |= addArgumentAttrsFromCallsites(*F);
 
     // Functions that are readonly (or readnone) and nounwind and don't return
     // a value can't capture arguments. Don't analyze them.
     if (F->onlyReadsMemory() && F->doesNotThrow() &&
         F->getReturnType()->isVoidTy()) {
       for (Function::arg_iterator A = F->arg_begin(), E = F->arg_end(); A != E;
            ++A) {
         if (A->getType()->isPointerTy() && !A->hasNoCaptureAttr()) {
           A->addAttr(Attribute::NoCapture);
           ++NumNoCapture;
           Changed = true;
for (Function::arg_iterator A = F->arg_begin(), E = F->arg_end(); A != E;
         Self.insert(&*A);
         Attribute::AttrKind R = determinePointerReadAttrs(&*A, Self);
         if (R != Attribute::None) {
           A->addAttr(R);
           Changed = true;
           R == Attribute::ReadOnly ? ++NumReadOnlyArg : ++NumReadNoneArg;
         }
       }
     }
   }
 
   // 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
   // made.  If the definition doesn't have a 'nocapture' attribute by now, it
   // captures.

llvm/test/Transforms/FunctionAttrs/writeonly.ll

+; RUN: opt < %s -O1 -S | FileCheck %s
+; RUN: opt < %s -O2 -S | FileCheck %s
+; RUN: opt < %s -O3 -S | FileCheck %s
+
+%_type = type <{ i8*, i8, i8, i8, i8 }>
+
+@d = internal global %_type <{ i8* null, i8 1, i8 13, i8 0, i8 -127 }>, align 8
+
+; CHECK: define void @foo(i32* nocapture writeonly %.aaa)
+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, %_type* @d, i32 0, i32 3)
+  ret void
+}