This is an archive of the discontinued LLVM Phabricator instance.

[FunctionAttrs] Remove readonly and writeonly assertion
ClosedPublic

Authored by luke on Apr 16 2019, 1:24 AM.

Details

Summary

There are scenarios where mutually recursive functions may cause the SCC
to contain both read only and write only functions. This removes an
assertion when adding read attributes which caused a crash with a the
provided test case, and instead just doesn't add the attributes.

Diff Detail

Repository
rL LLVM

Event Timeline

luke created this revision.Apr 16 2019, 1:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 16 2019, 1:24 AM
jdoerfert accepted this revision.Apr 16 2019, 5:50 AM

LGTM

I hope D60076 doesn't have the same problem ;)

This revision is now accepted and ready to land.Apr 16 2019, 5:50 AM
luke added a comment.Apr 17 2019, 8:09 AM

Hopefully not!
Also just a heads up, I don't have commit access to land this

luke added a comment.Apr 26 2019, 6:45 AM

@jdoerfert Can you commit this?

Hopefully not!
Also just a heads up, I don't have commit access to land this

Can you please rebase the patch?

Thanks. I wanted to land it for you, but since I dont use monorepo now, it does not apply well for me.

Maybe @jdoerfert could land it for you?

This is already accepted. Do you want me to commit this on behalf of you?

luke added a comment.Jul 15 2019, 8:39 AM

@vivekvpandya if possible, please

vivekvpandya added inline comments.Jul 15 2019, 9:36 AM
llvm/test/Transforms/FunctionAttrs/read-write-scc.ll
20 ↗(On Diff #209786)

while running check-all after your patch I am getting this lit test failed.
I am getting string
attributes #0 = { nofree nounwind }
could you please check this against tot?

This revision was automatically updated to reflect the committed changes.

I fixed the test and committed this.