Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
- Build Status
Buildable 44518 Build 45851: arc lint + arc unit
Event Timeline
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? |
llvm/docs/LangRef.rst | ||
---|---|---|
1868 | My problem/confusion was with the term Instruction here. These are (only) Call Site Attributes, correct? |
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? |
llvm/docs/LangRef.rst | ||
---|---|---|
1904 | reconducible? |
- I have changed the title of the section from Instruction Attributes to Call Site Attributes, as suggested by @jdoerfert.
- I have addressed the comments from @andwar.
llvm/docs/LangRef.rst | ||
---|---|---|
1904 | bha, I made up an English word. I replaced it with associated. |
I have rephrased the first paragraph to avoid mentioning the the term scalar function.
llvm/docs/LangRef.rst | ||
---|---|---|
1889 |
This sounds a bit odd:
|
Sorry for my absence on this.
llvm/docs/LangRef.rst | ||
---|---|---|
1871 | I'd word it something like this:
| |
1878 |
|
Address review from @jdoerfert.
llvm/docs/LangRef.rst | ||
---|---|---|
1878 |
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?
The aarch64 and x86 ones are linked below. For LLVM, this piece of docs is to be considered the mangling doc.
I'll use [ for optional instead of {. |
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. |
- Make sure that the attribute can be specified only for CallInst.
- Use [ and ] instead of { and } for optional tokens in the syntax of the attribute.
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. |
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.
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!
My problem/confusion was with the term Instruction here. These are (only) Call Site Attributes, correct?