Page MenuHomePhabricator

[AssumeBundle] Add documentation for the operand bundles of an llvm.assume
ClosedPublic

Authored by Tyker on Feb 7 2020, 3:17 AM.

Details

Summary

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.

Diff Detail

Event Timeline

Tyker created this revision.Feb 7 2020, 3:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 7 2020, 3:17 AM

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."
->
"makes it easy to identify uses in an :ref:llvm.assume <int_assume>. This then simplifies and improves heuristics, e.g., for use "use-sensitive" optimizations."

Tyker updated this revision to Diff 243447.Feb 9 2020, 6:54 AM
Tyker marked 4 inline comments as done.
Tyker retitled this revision from [AssumeBundle] Add documentation for assume operand bundles to [AssumeBundle] Add documentation for the operand bundles of an llvm.assume.
Tyker edited the summary of this revision. (Show Details)

tried to fix/improve formulations

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
we might also reference the regular assume here.

2139

Even if the assumed property can be encoded as a boolean value like `nonnull`.

has benefits *or* can still have benefits

lebedev.ri added inline comments.
llvm/docs/LangRef.rst
2125–2128

It may be good to add a footnote that the arguments needn't be constants (or are they?)

jdoerfert added inline comments.Feb 9 2020, 11:00 AM
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.

jdoerfert added inline comments.Feb 9 2020, 11:17 AM
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?

Tyker updated this revision to Diff 243909.Feb 11 2020, 10:06 AM
Tyker marked 13 inline comments as done.

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.
do they exist ?

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:
call void @llvm.assume(i1 true) ["align"(i32* %val, i32 8), "align"(i32* %ptr, i32 %alignment)]

2145

. If -> , if

2149

"value like `nonnull. Using" -> "value , like nonnull`, using"

17603–17604

I think it is better this way, explicit is good.

Tyker updated this revision to Diff 244690.Feb 14 2020, 9:15 AM
Tyker marked 4 inline comments as done.

addressed comments.

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
since the assumptions hold for the rest of the program.

2141

which call location? The one that returns %cond or %val? Should be made explicit.

jdoerfert accepted this revision.EditedFeb 15 2020, 7:56 PM

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.
[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

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.

2141

Again, we need to say: the `llvm.assume` call location

This revision is now accepted and ready to land.Feb 15 2020, 7:56 PM
Tyker marked an inline comment as done.Feb 17 2020, 1:47 PM
Tyker added inline comments.
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.

Tyker updated this revision to Diff 245034.Feb 17 2020, 1:49 PM
Tyker edited the summary of this revision. (Show Details)

addressed comments.

removed non-constant argument support as it isn't actually supported.

jdoerfert accepted this revision.Feb 18 2020, 12:34 PM

addressed comments.

removed non-constant argument support as it isn't actually supported.

Fine with me for now.

This revision was automatically updated to reflect the committed changes.