This is an archive of the discontinued LLVM Phabricator instance.

[LangRef] Add memory attribute
ClosedPublic

Authored by nikic on Oct 10 2022, 8:59 AM.

Details

Summary

This adds the LangRef wording for the memory attribute proposed at https://discourse.llvm.org/t/rfc-unify-memory-effect-attributes/65579.

The old attributes are not removed from LangRef until the migration is finished.

Diff Detail

Event Timeline

nikic created this revision.Oct 10 2022, 8:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 10 2022, 8:59 AM
Herald added a subscriber: jdoerfert. · View Herald Transcript
nikic requested review of this revision.Oct 10 2022, 8:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 10 2022, 8:59 AM

I'm pretty happy with the proposed spelling.

llvm/docs/LangRef.rst
1733–1735

Since this is a new kind of "composite" attribute, it should be clearly stated:

  • What the memory effects are if the attribute is missing entirely (presumably: all memory effects are possible, unless one of the legacy attributes is present)
  • How attributes compose if you have one on the call instruction *and* on the callee (presumably: the possible memory effects are the *intersection* of memory effects given by the different attributes)
1748

*additionally

nikic updated this revision to Diff 466741.Oct 11 2022, 2:25 AM
nikic marked 2 inline comments as done.

Mention default (all effects) and how effects for a call are derived from call-site and function attributes.

Do we need two spelling "`memory() or memory(none)`:"?

We should talk about call sites not calls, as we refer to them as "call site attributes".

llvm/docs/LangRef.rst
1750

There is no explicit "other" example, if we introduce the keyword we should use it. If we don't introduce the keyword we should not specify it as `other` but rather spell out non specified location kind means all non-specified locations kinds.

1772

intersection ... union ... does at best read confusing. Either we need to "union" both sides first or we just don't talk about that and simply write:

The memory effects of a call is the intersection of the call-site
   ``memory`` attribute with the function ``memory`` attribute and any effects implied by operand bundles.
nikic added a comment.Oct 12 2022, 9:01 AM

Do we need two spelling "memory() or memory(none):"?

Nope. I'll drop the memory() spelling.

llvm/docs/LangRef.rst
1772

What I mainly wanted to highly here is that the result is CallSiteEffects & (FuncEffects | BundleEffects), rather than other combinations, such as (CallSiteEffects & FuncEffects) | BundleEffects.

Maybe I should just write that out in code form, rather than make people try to parse English?

nikic updated this revision to Diff 467433.Oct 13 2022, 4:07 AM

Drop memory() spelling, add other example, use code to explain intersection behavior.

jdoerfert added inline comments.Oct 13 2022, 3:29 PM
llvm/docs/LangRef.rst
1767

I'm still not super happy with this scheme. other is all the location kinds we do not explicitly enumerate at at the current time, which is subject to change. At the same time we have an implicit "default" location kind which we don't even list in the "possible memory location kinds" above. So, we have two ways to refer to things not specified explicitly and they mean different things, that's confusing. Long story short, do we really need other?

1772

Maybe still add a sentence:
Thus, the call site annotations take precedence over the potential effects described by either the function annotation or the operand bundles.

nikic updated this revision to Diff 467776.Oct 14 2022, 7:44 AM

Remove other location kind, only allow specifying it via the default.

nikic marked 3 inline comments as done.Oct 14 2022, 7:45 AM
nikic added inline comments.
llvm/docs/LangRef.rst
1767

Okay, I went ahead and dropped other. I don't really see a use for it, and I agree that having both other and the default is somewhat confusing.

nikic marked an inline comment as done.
xbolva00 added a subscriber: xbolva00.EditedOct 14 2022, 8:06 AM

Just FYI,

https://github.com/llvm/llvm-project/issues/54312 :

I would recommend not to add size to the existing ones but instead replace them as whole. That said, mapping all but the size argument to existing ones would be great and pretty straight forward (IMHO).

What do you think, should 'memory' attribute take arg-index and size-index into account to support use cases like access attribute in C?

nikic added a comment.Oct 14 2022, 8:28 AM

Just FYI,

https://github.com/llvm/llvm-project/issues/54312 :

I would recommend not to add size to the existing ones but instead replace them as whole. That said, mapping all but the size argument to existing ones would be great and pretty straight forward (IMHO).

What do you think, should 'memory' attribute take arg-index and size-index into account to support use cases like access attribute in C?

I don't think so. The memory attribute describes the memory effects of the whole call/function. We have separate attributes that apply to individual arguments.

jdoerfert accepted this revision.Oct 14 2022, 10:48 AM

I like the wording now, thanks for the changes. Others should approve as well though.

Just FYI,

https://github.com/llvm/llvm-project/issues/54312 :

I would recommend not to add size to the existing ones but instead replace them as whole. That said, mapping all but the size argument to existing ones would be great and pretty straight forward (IMHO).

What do you think, should 'memory' attribute take arg-index and size-index into account to support use cases like access attribute in C?

I don't think so. The memory attribute describes the memory effects of the whole call/function. We have separate attributes that apply to individual arguments.

I agree. Argument access information should be attached to the arguments.
So __attribute__ ((access (read_only, 2, 3))) void* memcpy (void*, const void*, size_t); would become

declare i8* memcpy(i8* access(write[0:arg(2)]) %dst, i8* access(read[0:arg(2)]) %src, i32 %size) memory(argmem: readwrite)

And we should do this after.

This revision is now accepted and ready to land.Oct 14 2022, 10:48 AM
This revision was automatically updated to reflect the committed changes.