Page MenuHomePhabricator

[llvm][docs] LangRef for IR attribute `vector-function-abi-variant`.
ClosedPublic

Authored by fpetrogalli on Jan 15 2020, 11:58 AM.

Diff Detail

Event Timeline

fpetrogalli created this revision.Jan 15 2020, 11:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 15 2020, 11:58 AM

What are instruction attributes? How would that look like? Is there a list discussion?

What are instruction attributes? How would that look like? Is there a list discussion?

Hi @jdoerfert,

I thought we all agreed on this attribute by the fact we have implemented it - right now we have read/write functions for such attribute: https://reviews.llvm.org/D69976, and it is used in https://reviews.llvm.org/D72734

I couldn't find a section in the LangRef to describe it (it is not a function attribute) and decided to start a new one. I am happy to discuss to fall back on llvm-dev if you think I did something wrong.

Francesco

@fpetrogalli Thank you for working on this - just a few nits from me!

@jdoerfert I also assume that we have already agreed on this. Happy to fall back to llvm-dev if you feel otherwise.

llvm/docs/LangRef.rst
1876

function -> functions?

1877

scalar function -> scalar function that would otherwise we invoked by the call instruction? Maybe that's too verbose.

1883

correspondent -> corresponding ?

1889

the optional -> the optional name?

1890

Why would one provide a custom name? Perhaps it's worth expanding here.

1892

Declaration is a _necessary_ condition, definition is a _sufficient_ condition. Wouldn't declaration cover both?

1903

This is a bit confusing - with `<isa> = __LLVM__, which token specification one should use? The one for X86 or Arm? Could you clarify what <isa> == __LLVM__` means in practical terms?

jdoerfert added inline comments.Jan 16 2020, 10:10 AM
llvm/docs/LangRef.rst
1868

My problem/confusion was with the term Instruction here. These are (only) Call Site Attributes, correct?

fpetrogalli marked 8 inline comments as done.

Update text according to review.

llvm/docs/LangRef.rst
1868

Yes, the attribute I am describing here is an attribute that can be attached to a CallInst. I didn't know how to title the section, I am happy to suggestions.

1877

Hum, I am not convinced by this change. What's behind your suggestion?

andwar added inline comments.Jan 17 2020, 2:06 AM
llvm/docs/LangRef.rst
1877

You refer to the scalar function, but no scalar function is introduced beforehand. I know what scalar function you mean, but IMO it's not obvious from the context. But I'm a non-native speaker, so maybe just me :)

1912

For all targets (x86, Arm and Internal LLVM) - x86, Arm, internal LLVM are not all targets. Did you mean all targets for which this attribute is currently supported?

andwar added inline comments.Jan 17 2020, 2:09 AM
llvm/docs/LangRef.rst
1904

reconducible?

fpetrogalli marked 3 inline comments as done.
  1. I have changed the title of the section from Instruction Attributes to Call Site Attributes, as suggested by @jdoerfert.
  2. I have addressed the comments from @andwar.
fpetrogalli marked an inline comment as done.Jan 17 2020, 7:27 AM
fpetrogalli added inline comments.
llvm/docs/LangRef.rst
1904

bha, I made up an English word. I replaced it with associated.

fpetrogalli marked an inline comment as done.

I have rephrased the first paragraph to avoid mentioning the the term scalar function.

fpetrogalli marked 2 inline comments as done.Jan 17 2020, 8:07 AM
andwar added inline comments.Jan 20 2020, 1:30 AM
llvm/docs/LangRef.rst
1889

When present, the optional token `(<vector_redirection>) informs the compiler that a custom name is provided instead of the standard one _ZGV<isa><mask><vlen><parameters>_<scalar_name>`

This sounds a bit odd:

  • a custom name is provided instead of the standard one -> a custom name is provided in addition to the standard one? As far as the attribute is concerned, the custom name is something that's additional/optional, right. AFAIK, it's not there _instead_ of the standard name (but indeed, later will be used _instead_ of the standard name). Maybe worth claryfying?
  • the standard one _ZGV<isa><mask><vlen><parameters>_<scalar_name> -> here you repeat the definition of the standard name, which was already specified in the previous sentence. Maybe just remove `_ZGV<isa><mask><vlen><parameters>_<scalar_name>`?
fpetrogalli marked an inline comment as done.

Update last round of review from @andwar.

Adding some text here to convince phabricator to mark a comment as done. :)

Sorry for my absence on this.

llvm/docs/LangRef.rst
1871

I'd word it something like this:

In addition to function attributes the following call site only attributes are supported:

1878
  1. Only CallInst or call site?
  2. Do we have a mangling doc entry that we could link to?
  3. Do the { and ( belong to the token stream? Maybe [ is better to indicate optional things?
fpetrogalli marked 2 inline comments as done.

Address review from @jdoerfert.

llvm/docs/LangRef.rst
1878

Only CallInst or call site?

I am not sure what the distinction is. For now we are in need of it only for CallInst. What would be a call site?

Do we have a mangling doc entry that we could link to?

The aarch64 and x86 ones are linked below. For LLVM, this piece of docs is to be considered the mangling doc.

Do the { and ( belong to the token stream? Maybe [ is better to indicate optional things?

I'll use [ for optional instead of {.

jdoerfert added inline comments.Jan 21 2020, 3:12 PM
llvm/docs/LangRef.rst
1878

Call site is a term (and for now a class llvm::CallSite) that describes CallInst, InvokeInst, CallBr (I think), and later potentially other call like instructions. If these attributes can be placed on an InvokeInst which I was expecting, we need to say call site or also mention InvokeInst. If they cannot, we should mention that explicitly I think.

Also, if you refer to CallInst we need the ticks and a link to the definition in the lang ref.

fpetrogalli marked an inline comment as done.
  1. Make sure that the attribute can be specified only for CallInst.
  2. Use [ and ] instead of { and } for optional tokens in the syntax of the attribute.
fpetrogalli marked 2 inline comments as done.Jan 22 2020, 9:28 AM
fpetrogalli added inline comments.
llvm/docs/LangRef.rst
1878

My understanding of the callbr and invoke instructions is that they cannot be vectorized, as they both have the following sentence in the description: [...] [the instruction] causes control to transfer to a specified function [...].

I don't think this is compatible with the concept of concurrent execution of the functions on the lanes of the vector that is implied by a vector function.

For this reason, I have modified the text to make sure it applies only to CallInst.

I'm fine with this but I want to make sure ppl see this before it goes in.

fpetrogalli marked an inline comment as done.

Fix a typo in the name of the attribute (remove trailing s).

I'm fine with this but I want to make sure ppl see this before it goes in.

Sure! Thank you for looking into this. Francesco

Nit: You might want to add that list order does not imply preference (it's logically a set) and that the compiler is free to pick any listed vector function of its choosing.
Question: how does this interact with vector function metadata attached to a function declaration (if at all), eg does the call site list override that of the function?
Otw, LGTM.

Specify that the order of the list in the attribute is not important.

Nit: You might want to add that list order does not imply preference (it's logically a set) and that the compiler is free to pick any listed vector function of its choosing.

Done

Question: how does this interact with vector function metadata attached to a function declaration (if at all), eg does the call site list override that of the function?

Hum - I haven't really thought about that. I think that more than "override", the local list should extend the one attached to the function. At the end, what we need to prevent is that the call sites don't mix up their respective list (as they may come from different compilation unit being merged).

My preference would be to forbid the attribute at all for functions, and leave it only for callinst, as vectorization is instantiated only when a call is seen. Any concerns on this approach?

Otw, LGTM.

Thanks for your review, Simon!

This revision is now accepted and ready to land.Jan 25 2020, 12:25 PM
fpetrogalli retitled this revision from [llvm][docs] LangRef for IR attribute `vector-function-abi-variants`. to [llvm][docs] LangRef for IR attribute `vector-function-abi-variant`..Jan 27 2020, 2:01 PM
simoll accepted this revision.Jan 28 2020, 12:57 AM

Thx!

This revision was automatically updated to reflect the committed changes.