[PATCH] D123531: [GlobalsModRef][FIX] Ensure we honor synchronizing effects of intrinsics

Fri Apr 15 11:08:47 PDT 2022

tra added a comment.

In D123531#3454133 <https://reviews.llvm.org/D123531#3454133>, @jdoerfert wrote:

> OK. But I'm still not sure where you want me to document what exactly. What I "changed" here is:
>
> - Intrinsics can execute arbitrary code if they are not annotated with `nocallback`, and

Does it imply that we can not trust `Intr*Mem` attributes? If that's the case, perhaps we should implement some sort of sanity check for the attributes to report nonsensical combinations.

If `Intr*Mem` take precedence, then it should be documented. E.g. if we have `IntrArgMemOnly`, then we're allowed to assume that the intrinsic will only touch specific arg only.

Also, currently Intrinsics.td says this:

  //... 'has no other side effects'
  // language of the IntrNoMem, IntrReadMem, IntrWriteMem, and IntrArgMemOnly
  // intrinsic properties.`

This appears to contradict your assertion that lack of `nocallback` implies execution of arbitrary code which in turn implies arbitrary side effects.

I can't tell from reading `Intrinsics.td` what side effects we expect from an intrinsic with only `IntrNoMem`. Do we now need to specify `IntrNoMem`,`nosync`, `nocallback` in order to have a truly no-side-effects intrinsic?

> - Intrinsics can make effects of other "threads of execution" visible if they are not annotated with `nosync`.

Similarly here. `nosync` gives us a guarantee that no synchronization takes place, but without it, can we believe `IntrReadMem` that we do not write anything? Or do we need `nosync` for `IntrReadMem` to have an effect?

> For me that was already a given because we don't have the opposite written anywhere (as far as I know).

The problem with `no*` attributes is that a person who looks at the intrinsics that do not have them would not know that lack of an attribute may affect the meaning of the explicitly specified ones. 
It's hard to ask questions about something one may not even be aware of. Hence my harping on  documenting the interaction between these attributes where developers will see it.

> I could put the two points explicitly here: https://llvm.org/docs/LangRef.html#intrinsic-functions, would that be a good place?

LangRef does not seem to say anything specifically about intrinsics' attributes (though they do map to function attributes). 
It also does not mention `nocallback` at all.

I was thinking of augmenting `Intrinsics.td` as that's where developers who implement intrinsics would look for the reference info.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123531