[PATCH] D68484: [PATCH 01/38] [noalias] LangRef: noalias intrinsics and noalias_sidechannel documentation.

Mon Oct 7 14:41:54 PDT 2019

jdoerfert added a comment.

Thank you very much for working on this and putting all this into motion!

I started to look at this patch in isolation but with the rough idea of the approach in mind.
I did add various comments, from small wording changes to proposals on how we conceptually describe things.
Given that many comments would be repeated for each intrinsic, I stopped after the first and want to see what people think.

================
Comment at: llvm/docs/LangRef.rst:16232
+The intrinsics can also be used to specify alias assumptions that are
+not restrict based.
+
----------------
Nit:
> of `load` and `store` instructions.
The "of" sounds weird to me.

---
> The documentation below explains how it works, with ``restrict`` in mind.
I personally dislike sentences like this and would just remove it. 
> The intrinsics can also be used to specify alias assumptions that are not restrict based.
Arguably that is always true. The section describes the semantics of the alias stuff and how that can be used to model `restrict`. It is implied that other things can be modeled as well.

================
Comment at: llvm/docs/LangRef.rst:16249
+
+Note: ``XXX`` is the encoding of the return type and the types of the arguments.
+
----------------
I think you mix the "templated" definition (`XXX`) with instantiations (`i8**`, `%struct.FOO*`, ...). I would prefer we pick either. Precedence says you replace `XXX` with the types of that instantiation.

================
Comment at: llvm/docs/LangRef.rst:16275
+allows optimization passes to adapt the normal pointer path, without impacting
+the knowledge about the *depends on* relationship.
+
----------------
> introduces alias assumptions 
plural vs singular
> in the normal computation path
this isn't a "known" term for me (see below)
> of a pointer and it will be opaque for most optimizations
this is the hope but it is questionable if it is true and why it is here

I would replace the first sentence with:
"The ``llvm.noalias`` intrinsic attaches alias assumptions to its first argument."

---

The whole pass thing and splitting comes to early (IMHO). I don't know yet what these intrinsics mean but I learn that they are transformed. That said, `llvm.side.noalias` is not described here.

================
Comment at: llvm/docs/LangRef.rst:16281
+the loop is unrolled. Otherwise the restrict scope could spill across
+iterations.
+
----------------
Nit: remove "is used to", just "identifies"
Nit: remove "exact" (what does it mean given that we actually move stuff around under the normal "as if" rules)

It's not "done inside" and loops are only one example of this. What you want to say is more general:
"Whenever a `llvm.noalias.decl` intrinsic is duplicated through code transformations, care must be taken to duplicate and uniquify the scopes and intrinsics. These steps are described in the following."
To be honest, I'm not sure if it makes sense to say something like that here already. 

================
Comment at: llvm/docs/LangRef.rst:16286
+or returned from a function. After inlining, the correct dependencies can still
+be propagated.
+
----------------
Not: stray "this" in 16284
The inlining sentence does not really clear up anything here, partially because we don't know what is happening.

================
Comment at: llvm/docs/LangRef.rst:16290
+pointer points to a blob of memory that contains restrict pointers. This allows
+to track the *based on* dependency when copying such blocks of memory.
+
----------------
remove "a blob of"

================
Comment at: llvm/docs/LangRef.rst:16301
+  Different ``%p.addr`` represent different restrict pointers, pointing to
+  disjunct objects.
+- ``p.objId``: a number that can be used to differentiate different *object P*
----------------
> Either a real object, a constant where the value is relative to 0 or ``null``.
There is a word missing and 0 is `null`.

================
Comment at: llvm/docs/LangRef.rst:16303
+- ``p.objId``: a number that can be used to differentiate different *object P*
+  when ``%p.addr`` is optimized away.
+- ``!p.scope``: metadata argument that refers to a list of alias.scope metadata
----------------
This seems odd, why introduce two things that do the same thing.

================
Comment at: llvm/docs/LangRef.rst:16306
+  entries that contains exactly one element. It represents the variable
+  declaration that contains one or more restrict pointers.
+- ``%p.decl``: points to the ``@llvm.noalias.decl`` intrinsic associated with
----------------
"entries with a single element each."
> It represents the variable declaration that contains one or more restrict pointers.
I do not understand this sentence.

================
Comment at: llvm/docs/LangRef.rst:16310
+- ``%p.alloca``: points to the alloca associated with the declaration of a
+  restrict variable
+- ``%side.p``: the noalias_sidechannel associated with ``%p``.
----------------
For both items above:
No need for "points to".
> a restrict variable.
Maybe more specific: "the restrict pointer `%p`."

================
Comment at: llvm/docs/LangRef.rst:16314
+- ``%p.block``: the address of a block in memory or on the stack is associated
+  with a variable that contains at least one restrict pointer.
+- ``!p.indices``: metadata argument that refers to a list of metadata
----------------
Maybe:
"the address of an object with at least one restrict pointer constituent.

================
Comment at: llvm/docs/LangRef.rst:16317
+  references. Each reference points to a metadata array of indices. At the
+  specified location, a restrict pointer is located. A '-1' indicates any index.
+
----------------
I did not understand the above wording.

================
Comment at: llvm/docs/LangRef.rst:16334
+The restrictness of a restrict pointer ``%p``, as given by the
+``llvm.side.noalias`` intrinsic, can be expressed by following arguments:
+
----------------
Nit: "by the following"

================
Comment at: llvm/docs/LangRef.rst:16338
+- ``p.objId``: an extra number
+- ``!p.scope``: a declaration scope, related to the variable declaration.
+
----------------
"an extra number" is not helpful
" related to " -> "describing"/"identifying"

================
Comment at: llvm/docs/LangRef.rst:16353
+  identical to any other tuple, unless it can be proven that their ``%p.addr``
+  are disjunct.
+
----------------
"related to" -> "referencing" or "scoped in"

================
Comment at: llvm/docs/LangRef.rst:16483
+
+      declare i8* @llvm.noalias.decl.XXX(<type>* %p.alloca, i32 <p.objId>, metadata !p.scope)
+
----------------
I mentioned that before, the lady of the lake says XXX is specialized here:
https://llvm.org/docs/LangRef.html#llvm-memcpy-intrinsic

(do we add attributes here? if so, we need attributes here, e.g., `nocapture`).

================
Comment at: llvm/docs/LangRef.rst:16492
+certain ``alloca`` is associated to an object that contains one or more
+restrict pointers.
+
----------------
I dislike the loop body and alloca sentence because they do not convey information. To be honest, I don't know what the second sentence is saying. What I would prefer is to say something like:
"The intrinsic identifies the scope of the restrict restrict pointer through a virtual side-effect that ensures the control dependences are preserved. This virtual side-effect will also keep allocations alive and explicit."
(I guessed what you want to say wrt. alloca)

(allocas and mallocs should not be treated differently anyway, we have a heap-2-stack transformation now and for the purpose of this discussion there should not be a difference anyway)

================
Comment at: llvm/docs/LangRef.rst:16496
+not really represent a value. It is merely used to track a dependency on the
+declaration.
+
----------------
The above reads funny, maybe:
"The returned value is a handle to track dependences on the declaration. There is no explicit relationship to the value of the arguments."
Also, why do we want an `i8*` then? We have `tokens` and we have `i32`, I'd prefer either over an `i8*` which is more confusing in this context full of `i8*` that are actually pointers (IMHO).

================
Comment at: llvm/docs/LangRef.rst:16505
+
+The second argument ``p.objId`` is an integer representing an object id.
+
----------------
1) I would love to remove this duplication, is that possible?
2) Why do we need to talk about `alloca` and "optimized away"? Can't we say:
"The first argument `%p.alloca` points to an object in memory with one or more restrict pointers constituents or `null`."

================
Comment at: llvm/docs/LangRef.rst:16509
+metadata references. The format is identical to that required for ``noalias``
+metadata. This list must have exactly one element.
+
----------------
Is it a list with one element or a list with entries that have one element each? What I read earlier sounded different from what I read here.

================
Comment at: llvm/docs/LangRef.rst:16518
+the loop is unrolled. Otherwise the restrict scope could spill across
+iterations.
+
----------------
Copy and paste from somewhere above. I'd avoid duplication if possible in favor of references.

================
Comment at: llvm/docs/LangRef.rst:16524
+the relationship between those intrinsics and the actual variable declaration
+visible.
+
----------------
The above sentence is broken somewhere (I think). Maybe make it two.

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

https://reviews.llvm.org/D68484