Page MenuHomePhabricator

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

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

Details

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

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
1–3 ↗(On Diff #188436)

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 ↗(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 added inline comments.Apr 3 2019, 11:12 AM
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 marked an inline comment as done.Apr 3 2019, 11:52 AM
anhtuyen added inline comments.
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 ?

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 ↗(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 updated this revision to Diff 217692.Aug 28 2019, 11:24 AM
anhtuyen retitled this revision from LLVM: Optimization Pass: Function Attribute: Fix error caused by adding incompactible attribute to WriteOnly attribute of pointer argument to LLVM: Optimization Pass: Remove conflicting attribute, if any, before adding new read attribute to an argument.
anhtuyen edited the summary of this revision. (Show Details)

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 added inline comments.Aug 28 2019, 1:23 PM
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 marked 2 inline comments as done.Aug 28 2019, 1:52 PM
anhtuyen added inline comments.
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 added inline comments.Aug 29 2019, 1:36 PM
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 updated this revision to Diff 217996.Aug 29 2019, 4:03 PM

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

anhtuyen marked 8 inline comments as done.Sep 9 2019, 10:20 AM

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!

anhtuyen marked an inline comment as done.Sep 9 2019, 10:20 AM
rnk accepted this revision.Sep 9 2019, 2:07 PM

lgtm

This revision is now accepted and ready to land.Sep 9 2019, 2:07 PM
This revision was automatically updated to reflect the committed changes.