[PATCH] D93039: Introduce llvm.noalias.decl intrinsic

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

In D93039#2448573 <https://reviews.llvm.org/D93039#2448573>, @jeroen.dobbelaere wrote:

> In D93039#2446719 <https://reviews.llvm.org/D93039#2446719>, @nikic wrote:
>
>> While the concept of the intrinsic is intuitive, I think we're missing something here in terms of how it is formalized. I have a bit of a hard time formulating the rules.
>
> The rules that I have in mind have more freedom. They are more like:
>
> - `@llvm.noalias.decl` is something like a barrier that indicates where a noalias scope originates
> - It should normally not be moved across block boundaries. (This is to avoid a declaration crossing a loop boundary)
> - Moving it inside a block should pose no problems.
> - When it is duplicated, care must be taken to see what to do with the noalias scope.
>   - cases like loop unrolling and loop rotation: scope must be duplicated
>   - cases like loop unswitching: no need to duplicate the scope
>   - it should also not block optimizing away an 'almost empty loop' (in that case, the empty loop should be replaced by the `@llvm.noalias.decl`)
> - instructions that have !alias.scope or !noalias metadata referring to the declared scope can be moved freely across the intrinsic.
> - A `@llvm.noalias.decl` can be omitted if there is no `!alias.scope` metadata referring to the scope.
>   - An `!alias.scope` usage, corresponds to a `@llvm.noalias/@llvm.ptr_provenance.noalias` usage in the full restrict version. When there are no more dependencies, the `@llvm.noalias.decl` intrinsic can be omitted.
> - A missing `@llvm.noalias.decl` inside a function can lead to undefined behavior after inlining (and loop unrolling).
> - Multiple `@llvm.noalias.decl` intrinsics in the same block, referring to the same scope can be merged.
> - Multiple `@llvm.noalias.decl` intrinsics in the same block, referring to a different scopes can also be merged (but I would not yet do this).

I think it is generally beneficial to make the rules stricter rather than laxer. The reason is that we can implement checks in the IR Verifier and then automatically find optimizations that break the invariants. For example, if we have a rule that says:

> If a noalias.decl for !scope is dominated by another noalias.decl for !scope, then the IR is malformed.

Then, performing loop unrolling without proper handling of alias scopes, will automatically result in a verifier error. Without the rule the IR remains valid, just incorrect. Similarly, a rule that says:

> If a noalias.decl for !scope exists in the function, then all uses of !scope must be dominated by the noalias.decl.

Then this will also automatically detect certain incorrect code motion optimizations. (I'm less sure about this one, but I think we generally already drop noalias metadata when moving loads upwards.)

Does this make some sense?



================
Comment at: llvm/docs/LangRef.rst:19466
+
+      declare i8* @llvm.noalias.decl.XXX(<type>* %p.alloca, i64 <p.objId>, metadata !p.scope)
+
----------------
jeroen.dobbelaere wrote:
> nikic wrote:
> > The `p.alloca` and `p.objId` arguments don't make a lot of sense without the remainder of the full restrict patch. The objId refers to a notion that doesn't exist in current LLVM.
> > 
> > I think it would be better to make the intrinsics just
> > ```
> > declare void @llvm.noalias.decl(metadata !p.scope)
> > ```
> > to start with. Adding additional arguments via AutoUpgrade should be straightfoward down the line.
> The initial recommendation during the AA tech call was to already add all the arguments even if they are not needed yet. I guess that omitting them for now could work. Do you know the exact policy for backwards compatibility ? (released vs unreleased)
Not sure on policy for unreleased versions, but it should be simple enough to autoupgrade it in either case.


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

https://reviews.llvm.org/D93039