Page MenuHomePhabricator

LLVM: Optimization Pass: Function Attribute: Fix error caused by adding incompactible attribute to WriteOnly attribute of pointer argument
Needs ReviewPublic

Authored by anhtuyen on Feb 26 2019, 12:41 PM.

Details

Summary

Update optimization pass to fix attribute deduction error

When determining read attributes for pointer arguments, the attribute readnone will be deduced/added to an argument even if the argument was explicitly marked writeonly. This deduction of the attribute caused an incompatible error. Despite its name, the readnone attribute has implied semantics about write operations: "If a function reads from or writes to a readnone pointer argument, the behavior is undefined." This explains why the two attributes readnone and writeonly are considered incompatible.

~The correct attribute to be added in this case should be nocapture~

This can be demonstrated via the following snippet.

$ 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.

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

Diff Detail

Repository
rL LLVM

Event Timeline

anhtuyen created this revision.Feb 26 2019, 12:41 PM
rnk added a comment.Feb 26 2019, 12:50 PM

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
2–4

Please just run functionattrs directly.

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.

anhtuyen retitled this revision from LLVM: Optimization Pass: Function Attribute: No read-attribute should be added if the argument is WriteOnly to LLVM: Optimization Pass: Function Attribute: nocapture should be added if the argument is WriteOnly.Feb 26 2019, 1:08 PM
anhtuyen edited the summary of this revision. (Show Details)
rnk added a comment.Feb 26 2019, 2:20 PM

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.

In D58694#1411343, @rnk 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.

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

anhtuyen updated this revision to Diff 193542.Apr 3 2019, 10:46 AM
anhtuyen retitled this revision from LLVM: Optimization Pass: Function Attribute: nocapture should be added if the argument is WriteOnly to LLVM: Optimization Pass: Function Attribute: Fix error caused by adding incompactible attribute to WriteOnly attribute of pointer argument.
anhtuyen edited the summary of this revision. (Show Details)

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 added inline comments.Apr 3 2019, 11:09 AM
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.

730

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 added inline comments.Apr 3 2019, 11:12 AM
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 marked an inline comment as done.Apr 3 2019, 11:52 AM
anhtuyen added inline comments.
llvm/lib/Transforms/IPO/FunctionAttrs.cpp
730

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

anhtuyen edited reviewers, added: jdoerfert; removed: majnemer.Apr 3 2019, 11:57 AM
jdoerfert added inline comments.Apr 3 2019, 12:21 PM
llvm/lib/Transforms/IPO/FunctionAttrs.cpp
730

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).