[PATCH] D68484: [PATCH 01/27] [noalias] LangRef: noalias intrinsics and ptr_provenance documentation.

[Slava Zakharin via Phabricator via llvm-commits]

vzakhari added inline comments.


================
Comment at: llvm/docs/NoAliasInfo.rst:41
+a function (function arguments and local variables), will get a noalias scope
+that is associate d to this variable declaration. The ``!noalias`` metadata is
+used to annotate every memory instruction in the function with the restrict
----------------
typo: `associate d`


================
Comment at: llvm/docs/NoAliasInfo.rst:362
+      %rpA.address = alloca i32*
+      %rpA.decl = call @llvm.noalias.decl %rpA.address, !metadata !10 ; declaration of a restrict pointer
+      store i32* %pA, i32** %rpA.address, !noalias !10
----------------
Please add parenthesis, so that it looks like a call instruction.  Same at line 365.


================
Comment at: llvm/docs/NoAliasInfo.rst:374
+load/stores are not aliasing, based on the ``noalias`` annotations. But, the
+added intrinsics must block optimizations. Later on we will see how the
+infrastructure is expanded to allow for optimizations.
----------------
Can you please expand on the meaning of "must block optimizations"?  Maybe list some (important) optimizations that are blocked and insert a link to `Optimization passes` section below for optimizations that are not blocked but must handle the scopes properly?


================
Comment at: llvm/docs/NoAliasInfo.rst:398
+When an optimization introduces a ``load``/``store`` without keeping the
+``ptr_provenance`` operand and the ``!noalias`` metadata, we fall back to the
+fail-safe *worst case*.
----------------
Should `and` here be `or/and`?

I think it might be useful to put an example somewhere that shows what happens when a pointer provenance cannot be proven not to be based on a restrict pointer.  For example:
```
void unknown();
extern int *gp1;
extern int *gp2;
void test(int *restrict p1) {
  gp1 = p1;
  unknown();
  for (int i = 0; i < 100; ++i)
    gp2[i] = p1[i] + 1;
}
```

As I understand, the store instruction inside the loop uses a non-restrict pointer (`gp2`) with unknown/missing provenance and since `p1` escapes, `gp2` might be based on `p1`.  Thus, the load and the store pontentially alias.  I would be great to see an explanation of the logic used to compute the `MayAlias` result here.


================
Comment at: llvm/docs/NoAliasInfo.rst:457
+      %rpA.address = alloca i32*
+      %rpA.decl = i8* call @llvm.noalias.decl i32* %rpA.address, !metadata !10 ; declaration of a restrict pointer
+      store i32* %pA, i32** %rpA.address, !noalias !10
----------------
Please add parenthesis for the calls.


================
Comment at: llvm/docs/NoAliasInfo.rst:603
+For convenience, each function can have its own ``unknown function`` scope
+specified by a ``noalias !UnknownScope`` metadata attribute on the function itself.
+
----------------
Can you please add some example(s)?  As I understand, this describes cases with global restrict pointer and with indirect loads of restrict pointers (e.g. `int *restrict *restrict`).


================
Comment at: llvm/docs/NoAliasInfo.rst:624
+  * ``%p.decl``: (when available), the ``@llvm.noalias.decl`` associated with the object
+  *  ``!Indices``: this refers to a metadata list. Each element of the list
+     refers to a set of indices where a restrict pointer is located, similar to
----------------
Do we need to represent the aggregate type as an extra argument to make it work with opaque pointers?


================
Comment at: llvm/docs/NoAliasInfo.rst:721
+.. note:: The ``Unknown Function Scope`` is a special scope that is attached
+          through ``!noalias`` metadata on a function defintion. It identifies
+          the scope that is used for *noalias* pointers for which the
----------------
Is it important that the unknown scope has the function scope as its "parent"?

```
!6 = distinct !{!6, !7, !"test: unknown scope"}
!7 = distinct !{!7, !"test"}
```


================
Comment at: llvm/docs/NoAliasInfo.rst:890
+
+And LLVM-IR code after optimizations: alias analysis found the the stores do not
+alias to each other and the values have been propagated.
----------------
typo: "the the"


================
Comment at: llvm/docs/NoAliasInfo.rst:1019
+* ``objId``: an ID that is associated to this declaration. *SROA* treats this as
+  an offset wrt to the original ``alloca``.
+* ``!Scope``: a single scope that is associated with this declaration.
----------------
Is this correct?


================
Comment at: llvm/docs/NoAliasInfo.rst:1250
+``!Indices`` points to a list of metadata. Each entry in that list contains a
+set of ``i32`` values, corresponding to the indices that would be past to
+``getelementptr`` to retrieve a field in the struct. When the ``i32`` value is
----------------
`past` -> `passed`?


================
Comment at: llvm/docs/NoAliasInfo.rst:1384
+
+
+References
----------------
Can you please add notes that `ScopedNoAliasAA` is the alias analysis that is using the `noalias` and `ptr_provenance` information?
Do you also have estimation of the complexity of `alias` queries with the new representation?  How is it affected by the number of scopes in the scope lists attached to the load/store, by the length of the provenance chain, etc.?


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

https://reviews.llvm.org/D68484