Page MenuHomePhabricator

Add writeonly IR attribute
ClosedPublic

Authored by nhaehnle on Apr 1 2016, 2:21 PM.

Details

Summary

This complements the earlier addition of IntrWriteMem and IntrWriteArgMem
LLVM intrinsic properties, see D18291.

Also start using the attribute for memset, memcpy, and memmove intrinsics,
and remove their special-casing in BasicAliasAnalysis.

Diff Detail

Repository
rL LLVM

Event Timeline

nhaehnle updated this revision to Diff 52426.Apr 1 2016, 2:21 PM
nhaehnle retitled this revision from to Add writeonly IR attribute.
nhaehnle updated this object.
nhaehnle added reviewers: reames, mehdi_amini.
nhaehnle added a subscriber: llvm-commits.
mehdi_amini edited edge metadata.Apr 1 2016, 2:37 PM

Skimmed through, looks OK in principle, please see inline comments.

include/llvm/IR/Function.h
291 ↗(On Diff #52426)

Comment needs an update (reads->write)

293 ↗(On Diff #52426)

The name would be more accurate if it was doNotReadMemory().
(I agree there is an unfortunate precedent with onlyReadsMemory()

include/llvm/IR/Intrinsics.td
77 ↗(On Diff #52426)

Not clear (again unfortunate wording in the original)

nhaehnle added inline comments.Apr 1 2016, 3:51 PM
include/llvm/IR/Function.h
293 ↗(On Diff #52426)

Do you mean doesNotReadMemory(), or is the departure from third person intentional? I'll certainly fix the comment.

include/llvm/IR/Intrinsics.td
77 ↗(On Diff #52426)

What's not clear about it? Do you just mean a rephrasing like: "The intrinsic does not read memory through the specified argument pointer."

Hmm, or is this about the aliasing? I.e. one could clarify "As long as the intrinsic does not have the IntrWriteMem or IntrWriteArgMem properties, the memory pointed to by the pointer may still be read from via a different but aliasing argument pointer or if the intrinsic is not marked as accessing memory only through argument pointers."

nhaehnle updated this revision to Diff 52444.Apr 1 2016, 4:35 PM
nhaehnle edited edge metadata.

onlyWritesMemory -> doesNotReadMemory

Clarify the comment on the WriteOnly intrinsic argument property.

No other comments on my side, let's wait for Philip's review.

reames requested changes to this revision.Apr 21 2016, 9:17 AM
reames edited edge metadata.

A bunch of minor comments. Seems close to going in. I'd have preferred you split the addition of the attribute from the intrinsic changes, but will not require you to split them now.

docs/LangRef.rst
1385 ↗(On Diff #52444)

may write to

include/llvm/IR/Attributes.td
163 ↗(On Diff #52444)

I'm pretty sure that order doesn't matter in this file. Given that, please put this near readonly.

include/llvm/IR/Intrinsics.td
378 ↗(On Diff #52444)

WriteOnly<0>, IntrArgMemOnly would be slightly more consistent.

lib/Analysis/AliasAnalysis.cpp
146 ↗(On Diff #52444)

minor: this is exclusive of the previous case. using an else if to indicate this here and all other similar cases would be mildly useful.

lib/Analysis/BasicAliasAnalysis.cpp
627 ↗(On Diff #52444)

Please leave this comment.

633 ↗(On Diff #52444)

Don't drop the intrinsic bit from the comment.

lib/IR/Attributes.cpp
272 ↗(On Diff #52444)

Here and a couple other places in C++ code: not order sensitive, please group with other memory attributes.

lib/IR/Verifier.cpp
1498 ↗(On Diff #52444)

Duplicate code?

This revision now requires changes to proceed.Apr 21 2016, 9:17 AM
nhaehnle updated this revision to Diff 55276.Apr 27 2016, 11:51 AM
nhaehnle edited edge metadata.
nhaehnle marked 4 inline comments as done.

Rebase on trunk (which involves upping some invalid bitcode tests) and
address review comments.

nhaehnle marked 4 inline comments as done.Apr 27 2016, 11:51 AM

Hi Philip, thanks for the feedback. All comments are either reflected in the updated revisions or answered here.

include/llvm/IR/Attributes.td
163 ↗(On Diff #52444)

I also believe the order doesn't matter, but I didn't do that because everything else is _very_ thoroughly alphabetized. Note how ArgMemOnly and InaccessibleMem(OrArgMem)Only also aren't close together...

lib/IR/Attributes.cpp
272 ↗(On Diff #52444)

Done here, since there's already a precedent for non-alphabetization, but not in getAttrMask where there is a strict order by bit number...

lib/IR/Verifier.cpp
1498 ↗(On Diff #52444)

readnone/writeonly vs. readonly/writeonly, so no duplication as far as I can see. The whole sequence is full of similarities, but this is probably not the right place to clean it up.

nhaehnle marked an inline comment as done.May 9 2016, 3:02 PM

Ping?

arsenm added a subscriber: arsenm.Jun 16 2016, 2:26 PM
arsenm added inline comments.
lib/IR/Verifier.cpp
1485–1487 ↗(On Diff #55276)

I don't see a verifier test for this

nhaehnle edited edge metadata.Jun 17 2016, 3:27 PM
nhaehnle marked 10 inline comments as done.Jun 17 2016, 3:30 PM
nhaehnle marked an inline comment as done.Jun 17 2016, 3:31 PM
nhaehnle updated this revision to Diff 61160.Jun 18 2016, 2:52 AM
nhaehnle marked an inline comment as done.

Added a verifier test for {readnone, readonly} vs. writeonly clashes.
Extend the verifier check to flag incompatible attributes also for
function parameters.

This revision was automatically updated to reflect the committed changes.
filcab added a subscriber: filcab.Jul 12 2016, 6:15 AM
filcab added inline comments.
llvm/trunk/include/llvm/IR/Intrinsics.td
367

Shouldn't memset be WriteOnly<1>?

nhaehnle added inline comments.Jul 12 2016, 6:50 AM
llvm/trunk/include/llvm/IR/Intrinsics.td
367

This is an attribute on a function argument, the number is the number of the function argument. See also the various NoCapture attributes.

Ah, of course.
Thank you!
Filipe