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

[Jeroen Dobbelaere via Phabricator via llvm-commits]

jeroen.dobbelaere marked 4 inline comments as done.
jeroen.dobbelaere added a comment.

Thanks for all the feedback ! I added some explanations.



================
Comment at: llvm/docs/LangRef.rst:16249
+
+Note: ``XXX`` is the encoding of the return type and the types of the arguments.
+
----------------
jdoerfert wrote:
> 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.
> 
> 
That's true, but imho the full intrinsic name become very long, cluttering the display., For clarity I replaced the type encodings with XXX. This makes it easier to focus on the intrinsics and the actual arguments. I agree that this is not perfect.


================
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
----------------
jdoerfert wrote:
> This seems odd, why introduce two things that do the same thing.
The original idea was to treat '%p.addr' sometimes as a pointer to an object and sometimes as an offset. Later it needed to be separated: SROA first splits alloca's into multiple smaller alloca's. Each separate restrict pointer now points to its own alloca (%p.addr), and there is no place to put the offset. You can differentiate by splitting the p.scope, but that would imply duplicating scopes all over the place. The p.objId serves as a convenient and less costly solution to differentiate the pointers in this case.


================
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
----------------
jdoerfert wrote:
> "entries with a single element each."
> > It represents the variable declaration that contains one or more restrict pointers.
> I do not understand this sentence.
hmm. Not sure how to explain it further. What I want to say is (shown with an example:)
  int *restrict A;  // one !p.scope, one restrict pointer
  int *restrict B[10]; // another (single) !p.scope, ten restrict pointers
  struct FOO { int* restrict mA; int * mB; int* restrict mC; } C; // yet another !p.scope, 2 restrict pointers






================
Comment at: llvm/docs/LangRef.rst:16496
+not really represent a value. It is merely used to track a dependency on the
+declaration.
+
----------------
jdoerfert wrote:
> 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).
I think a token has to many restrictions (no PHI, no select). i32 might do. I didn't think too much about it and just settled on i8*.


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

https://reviews.llvm.org/D68484