Operand bundles on an llvm.assume allows representing
assumptions that an attribute holds for a certain value at a certain position.
Operand bundles enable assumptions that are either hard or impossible to
represent as a boolean argument of an llvm.assume.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Overall the test looks fine. I added a few comments, some rewording proposals and minor comments. We also need a proper commit message ;)
@fhahn Please take a look :)
llvm/docs/LangRef.rst | ||
---|---|---|
2113 | Nit: Maybe say: as a boolean argument of an llvm.assume`. | |
2119 | Nit: are no arguments, dot missing at the end. | |
2145 | 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. | |
2148 | "make detecting that the value is used in an :ref:llvm.assume <int_assume> easier." |
Looks better, I went over again and commented on minor language stuff. I'm not an expert either ;)
@fhahn what do you think?
llvm/docs/LangRef.rst | ||
---|---|---|
2113 | allows representing -> allow representing Operand bundles enable assumptions that are either hard or impossible to represent as a boolean argument of | |
2119 | It's not the path arguably but the (single) location of the llvm.assume | |
2134 | path -> call location | |
2136 | are incorrect -> are violated at runtime | |
2139 | Even if the assumed property can be encoded as a boolean value like `nonnull`. has benefits *or* can still have benefits |
llvm/docs/LangRef.rst | ||
---|---|---|
2125–2128 | It may be good to add a footnote that the arguments needn't be constants (or are they?) |
llvm/docs/LangRef.rst | ||
---|---|---|
2125–2128 | 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. |
llvm/docs/LangRef.rst | ||
---|---|---|
2125–2128 | Didn't read it properly. Good idea to say explicitly they might not be constant! Thx @lebedev.ri |
Added some additional reviewers, I think it would be good to have a few more people taking a look.
llvm/docs/LangRef.rst | ||
---|---|---|
2115 | 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. | |
2117 | nit: second also, what for attributes with more than 1 argument? | |
2119 | Clarify which path? | |
2125 | align isn't a string attribute I think. Shouldn't we use a non-string attribute here? | |
17597 | nit: maybe say something like 'More complex assumptions can be encoded as :ref:operand bundles <assume_opbundles>. | |
17603–17604 | Is this change needed? |
took comments in account.
llvm/docs/LangRef.rst | ||
---|---|---|
2115 | yeah this is easier to read. | |
2117 | they currently don't seem representable in IR by any of the *AttributeImpl. | |
2125 | 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. | |
2125–2128 | i edited an example so it is not always a constant is it clear enough this way ? | |
2145 | 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. | |
17603–17604 | this was to avoid confusion about which argument. but maybe it isn't needed. |
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.
llvm/docs/LangRef.rst | ||
---|---|---|
2119 | ", <attribute argument>" -> "[, <attribute argument>]" to make it optional | |
2125–2128 | 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: | |
2145 | . If -> , if | |
2149 | "value like `nonnull. Using" -> "value , like nonnull`, using" | |
17603–17604 | I think it is better this way, explicit is good. |
LGTM overall, just some nitpicks.
Something I would add is a link to the list of attributes (is this function-attributes?).
llvm/docs/LangRef.rst | ||
---|---|---|
2126 | which call location? | |
2134 | nitpick, maybe at this point -> from this point forward | |
2141 | which call location? The one that returns %cond or %val? Should be made explicit. |
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.
[EDIT: Please improve the commit message, e.g., by copying part of the lang ref definition.]
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 ...
llvm/docs/LangRef.rst | ||
---|---|---|
2126 | of the `llvm.assume` call location. | |
2134 |
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. | |
2141 | Again, we need to say: the `llvm.assume` call location |
llvm/docs/LangRef.rst | ||
---|---|---|
2125–2128 | it is possible to represent non-constant arguments using operand bundles. but the current query API doesn't handle that case. it is possible to deal with it but i think this will be at the cost of worse API and some performance. i think this should be disallowed at least for now. |
addressed comments.
removed non-constant argument support as it isn't actually supported.
Nit: Maybe say: as a boolean argument of an llvm.assume`.