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

[Tyker via Phabricator via llvm-commits]

Tyker added inline comments.


================
Comment at: llvm/docs/LangRef.rst:2145
+  not have every canonicalizations to deduce the property from any arbitrary
+  expressions that check the property.
+* Expressing the property using operand bundles make detecting that the value
----------------
jdoerfert wrote:
> It's a bit abstract. Maybe something along the lines of:
> 
> ```
> Encoding attributes as operand bundles removes the need for an instruction sequence that represents the property, e.g., `icmp ne i32* %p, null` for `nonnull`. These sequences can
> have negative effects due to the additional uses they introduce and the potential for them
> to be changed or combined with surrounding user code.
> ```
the use is mostly for the second bullet point. but i tried to reformulate to be more explicit.

that said maybe the 2 bullet point could be merged under there is not need for extra instructions. and then 2 bullet point on why extra expression are detrimental.


================
Comment at: llvm/docs/LangRef.rst:2115
+
+* The tag of the operand bundle is the attribute that can be assumed to hold.
+* The first argument if present is the value for which the attribute hold.
----------------
fhahn wrote:
> I think this list needs a bit of context, like 'An assume operand bundle has the form `"<tag>"(<holds for value>, <attribute argument>)`:
>  * ....
> 
> 
> Maybe also link to the section about attributes and clarify that only valid attributes are supported.
yeah this is easier to read.


================
Comment at: llvm/docs/LangRef.rst:2117
+* The first argument if present is the value for which the attribute hold.
+* The Second argument if present is an argument of the attribute.
+
----------------
fhahn wrote:
> nit: second
> 
> also, what for attributes with more than 1 argument?
they currently don't seem representable in IR by any of the *AttributeImpl.
do they exist ?



================
Comment at: llvm/docs/LangRef.rst:2125
+
+      call void @llvm.assume(i1 true) ["align"(i32* %val, i32 8)]
+
----------------
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.


================
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.
----------------
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 ?


================
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.
----------------
fhahn wrote:
> Is this change needed?
this was to avoid confusion about which argument. but maybe it isn't needed.


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

https://reviews.llvm.org/D74209