[PATCH] D74209: [AssumeBundle] Add documentation for the operand bundles of an llvm.assume

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

I have some more minor nits (below) but other than that I'm still fine with this. I think we have all the relevant parts flushed out by now and minor adjustments can be made as we go.



================
Comment at: llvm/docs/LangRef.rst:2125-2128
+      call void @llvm.assume(i1 true) ["align"(i32* %val, i32 8)]
+
+allows the optimizer to assume that ``%val`` has an alignment of at least 8
+at this point.
----------------
Tyker wrote:
> Tyker wrote:
> > fhahn wrote:
> > > jdoerfert wrote:
> > > > jdoerfert wrote:
> > > > > lebedev.ri wrote:
> > > > > > It may be good to add a footnote that the arguments needn't be constants (or are they?)
> > > > > I would prefer not to require that. As long as we come from `llvm::Attribute` that is the case but we derive information from other places (e.g., the user) eventually.
> > > > Didn't read it properly. Good idea to say explicitly they might not be constant! Thx @lebedev.ri 
> > > align isn't a string attribute I think. Shouldn't we use a non-string attribute here?
> > i edited an example so it is not always a constant is it clear enough this way ?
> align isn't a string attribute but tags in operand bundles need to be a string. so all attributes are encoded with there name as a strings.
It's especially interesting for attributes that take only a constant in their attribute for, e.g., align. Maybe extend the example to show both non-constant values for those and how tags can occur multiple times:
`call void @llvm.assume(i1 true) ["align"(i32* %val, i32 8), "align"(i32* %ptr, i32 %alignment)]`


================
Comment at: llvm/docs/LangRef.rst:17587
 
-The condition which the optimizer may assume is always true.
+The argument of the call is the condition which the optimizer may assume is
+always true.
----------------
Tyker wrote:
> fhahn wrote:
> > Is this change needed?
> this was to avoid confusion about which argument. but maybe it isn't needed.
I think it is better this way, explicit is good.



================
Comment at: llvm/docs/LangRef.rst:2119
+
+      "<tag>"(<holds for value>, <attribute argument>)
+
----------------
", <attribute argument>" -> "[, <attribute argument>]"  to make it optional


================
Comment at: llvm/docs/LangRef.rst:2145
+Just like for the argument of :ref:`llvm.assume <int_assume>`. If any of the
+provided guarantees are are violated at runtime the behavior is undefined.
+
----------------
`. If ` -> `, if`


================
Comment at: llvm/docs/LangRef.rst:2149
+``nonnull``. Using operand bundles to express the property can still have
+benefits:
+
----------------
"value like ``nonnull``. Using" -> "value , like ``nonnull``, using"


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

https://reviews.llvm.org/D74209