[PATCH] D135597: [LangRef] Add memory attribute

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/docs/LangRef.rst:1767
+      default access kind that covers ``other``, rather than enumerating all
+      currently supported location kinds.
+
----------------
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?


================
Comment at: llvm/docs/LangRef.rst:1772
+    ``memory`` attribute with the union of the function ``memory`` attribute
+    and any effects implied by operand bundles.
 ``minsize``
----------------
nikic wrote:
> jdoerfert wrote:
> > 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.
> > ```
> 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?
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.`


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D135597/new/

https://reviews.llvm.org/D135597