[PATCH] D74209: [AssumeBundle] Add documentation for the operand bundles of an llvm.assume
Johannes Doerfert via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Sat Feb 15 19:57:13 PST 2020
jdoerfert accepted this revision.
jdoerfert added a comment.
This revision is now accepted and ready to land.
In D74209#1876692 <https://reviews.llvm.org/D74209#1876692>, @nlopes wrote:
> LGTM overall, just some nitpicks.
> Something I would add is a link to the list of attributes (is this `function-attributes`?).
LGTM as well with some minor suggestions, and one clarification for @nlopes, below.
I think with the modifications we can go ahead and improve as we go.
The first paragraph has the word `attribute`. We can link to the two attribute lang ref sections (as we have both parameter and function attributes). Maybe:
... assumptions that a :ref:`function attribute <fnattrs>` or :ref:`parameter attribute <paramattrs>` holds ...
================
Comment at: llvm/docs/LangRef.rst:2126
+
+If there are no arguments the attribute is a property of the call location.
+
----------------
nlopes wrote:
> which call location?
of the ``llvm.assume`` call location.
================
Comment at: llvm/docs/LangRef.rst:2134
+
+allows the optimizer to assume that at this point ``%val`` has an alignment of
+at least 8 and ``%ptr`` has an alignment of at least ``%alignment``.
----------------
nlopes wrote:
> nitpick, maybe at this point -> from this point forward
> since the assumptions hold for the rest of the program.
> nitpick, maybe at this point -> from this point forward
> since the assumptions hold for the rest of the program.
Arguably not the ones that do not have an associated value, e.g., the `cold()` example below. All the ones that "bind" to a value do hold for the rest of the program though. But since that is "implied", and we have the other ones, I would not go there for now.
================
Comment at: llvm/docs/LangRef.rst:2141
+
+allows the optimizer to assume that the call location is cold and that ``%val``
+may not be null.
----------------
nlopes wrote:
> which call location? The one that returns `%cond` or `%val`? Should be made explicit.
Again, we need to say: the ``llvm.assume`` call location
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D74209/new/
https://reviews.llvm.org/D74209
More information about the llvm-commits
mailing list