llvm dev - Jun 2019 - [cfe-dev] [RFC] Expose user provided vector function for auto-vectorization.

Hi Francesco,

On 06/03, Francesco Petrogalli wrote:
> > On Jun 3, 2019, at 1:43 PM, Andrea Bocci <andrea.bocci at
cern.ch> wrote:
> > as a candidate future user of the proposed extension, I think I like
the simplified proposal better than the original RFC.
> > 
> > The only part of the syntax that I would find not very much
user-friendly is having to mangle the isa/mask/vlen/type list by hand.
> > 
> > Would a more C-like syntax be feasible ?
> 
> The syntax I proposed allows user to expose concepts like “linear”,
“uniform” and “aligned" parameters, as described in the `declare simd`
directive of OpenMP, which is what x86 and aarch64 Vector Function ABIs use to
mangle the names.
> 
> I agree with you that having to manually specify the mangling is a bit
cumbersome, I am open to suggestions.
> 
> An alternative approach was to make the actual string inside the attribute
to accept anything a `declare variant` in a `simd` context could accept, but
that wold probably raise the same problems that `declare variant` has raised.
> 
> In fact, on AArch64, a typical use of declare variant would be more
complicated than just specifying `simd` in the `context`, with `simdlen` and
`[not]inbranch`. It would require also to specify the `isa` trait from the
`device` set, and to use a `vendor` specific set to be able to list scalable
(i.e. vector-length agnostic) functions for SVE.
> 
> This would definitely look like implementing `declare variant`, as the
attribute would need to accept something like the following:
> 
> ```
> . . .
__attribute__(simd_variant(“vector_version”,“context={simd(simdlen(2),notinbranch”},device={isa(“simd")}))
> ```
> 
> Using the sub-string from the mangled name to me has the following
advantages:
> 
> 1. No need to deal with `declare variant`.
> 2. [...]
> 3. In terms of usability, there is no need to reinvent the wheel. The
> `declare variant` of OpenMP 5.0 is perfectly suited for what we are
> doing here, it is just much more than what it is needed for exposing
> user-defined function to the vectorizer. So, on the long term, I think
> we should definitely implement `declare variant`. Whatever attribute
> system we came up with this RFC, it will be easy to map the `simd`
> bits of `declare variant` to it.
I'm unsure about this. While I agree that avoiding to deal with
"declare
variant" will make this all much easier, you seem to agree that we will
eventually have to do that anyway. Adding something now that is a subset
of what we will need will put as in an awkward position. We will have to
unify the existing in-house concept with the standardized OpenMP way in
terms of semantics as well as implementation. I somehow doubt that this
will make things easier, especially if we later discover that the
in-house solution is somehow incompatible or not actually a subset.

To summarize: I will not oppose this effort but I would much rather see
us working towards "declare variant", a stronger concept we will have
to
support in the near future anyway.

Thanks,
  Johannes
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20190604/8f4fcd6b/attachment.sig>

> On Jun 4, 2019, at 12:23 PM, Doerfert, Johannes <jdoerfert at
anl.gov> wrote:
> 
> Hi Francesco,
> 
> On 06/03, Francesco Petrogalli wrote:
>>> On Jun 3, 2019, at 1:43 PM, Andrea Bocci <andrea.bocci at
cern.ch> wrote:
>>> as a candidate future user of the proposed extension, I think I
like the simplified proposal better than the original RFC.
>>> 
>>> The only part of the syntax that I would find not very much
user-friendly is having to mangle the isa/mask/vlen/type list by hand.
>>> 
>>> Would a more C-like syntax be feasible ?
>> 
>> The syntax I proposed allows user to expose concepts like “linear”,
“uniform” and “aligned" parameters, as described in the `declare simd`
directive of OpenMP, which is what x86 and aarch64 Vector Function ABIs use to
mangle the names.
>> 
>> I agree with you that having to manually specify the mangling is a bit
cumbersome, I am open to suggestions.
>> 
>> An alternative approach was to make the actual string inside the
attribute to accept anything a `declare variant` in a `simd` context could
accept, but that wold probably raise the same problems that `declare variant`
has raised.
>> 
>> In fact, on AArch64, a typical use of declare variant would be more
complicated than just specifying `simd` in the `context`, with `simdlen` and
`[not]inbranch`. It would require also to specify the `isa` trait from the
`device` set, and to use a `vendor` specific set to be able to list scalable
(i.e. vector-length agnostic) functions for SVE.
>> 
>> This would definitely look like implementing `declare variant`, as the
attribute would need to accept something like the following:
>> 
>> ```
>> . . .
__attribute__(simd_variant(“vector_version”,“context={simd(simdlen(2),notinbranch”},device={isa(“simd")}))
>> ```
>> 
>> Using the sub-string from the mangled name to me has the following
advantages:
>> 
>> 1. No need to deal with `declare variant`.
>> 2. [...]
>> 3. In terms of usability, there is no need to reinvent the wheel. The
>> `declare variant` of OpenMP 5.0 is perfectly suited for what we are
>> doing here, it is just much more than what it is needed for exposing
>> user-defined function to the vectorizer. So, on the long term, I think
>> we should definitely implement `declare variant`. Whatever attribute
>> system we came up with this RFC, it will be easy to map the `simd`
>> bits of `declare variant` to it.
> 
> I'm unsure about this. While I agree that avoiding to deal with
"declare
> variant" will make this all much easier, you seem to agree that we
will
> eventually have to do that anyway.
Please notice that by “avoiding to deal with `declare variant`” doesn’t mean
that I am not defining the attribute with the OpenMP directive `declare variant`
in mind. It just means that I don’t want (for now, int his RFC), discuss how to
implement _full_ support for `declare variant` for the feature discussed in this
RFC.
> Adding something now that is a subset
> of what we will need will put as in an awkward position.
Agree, and that’s why we need to craft it carefully to make sure we don’t get to
that point. Which is something that owe are definitely doing.
> We will have to
> unify the existing in-house concept with the standardized OpenMP way in
> terms of semantics as well as implementation.
Yes.
> I somehow doubt that this
> will make things easier, especially if we later discover that the
> in-house solution is somehow incompatible or not actually a subset.
> 
Agreed. The semantic of `declare variant` has more meanings than the `declare
simd` itself. But the specs state that the `simd(other-stuff)` trait that can be
passed to the `construct` set mirrors the semantic of the `declare simd`
directive.

“The simd trait can be further defined with properties that match the clauses
accepted by the declare simddirective with the same name and semantics.” (Page
52, lines 9-10).

The Vector Function ABI mangling scheme represent all the information that can
be passed via a `declare  simd` directive (more than that, because it encodes
also the target vector extension in the <isa> token). I can definitely
speak for the AArch64 one, but it seems the same method has been used for the
x86 one (@Hideki, @Xinmin, can you confirm?).

Therefore, we can all agree that the attribute `vector-variant`, as it has been
proposed, can be used to represent `declare simd` and `declare variant`, the
latter when used with the `simd` trait of the `construct` set.

The question that you raise is: “can we extend the information carried by
`vector-variant` to represent all the semantic of `declare variant`?“

Even if the answer to such question is no, is it really what we want to do?

I am just thinking at a situation where the `declare variant` is used not for a
single trait like `simd`, but also for other contexts.

```
#pragma omp declare variant(variant_foo)
match(construct={simd(simdlen(2),notinbranch),
another_trait(with_other_clause)}, device= {isa(TARGET_ISA)}, ...)
double foo(double);

Vector_double variant_foo(vector_double);
```

The pragma here is giving the following information:

1. The function `variant_foo` is obtained by `foo` with the following instance
of `declare simd`:

#pragma omp declare simd simdlen(2) notinbranch
double foo(double);

2. The function `variant_foo` is written using the instructions of the
TARGET_ISA specified in the `isa` trait of the `device` set.

Again, (I will never get tired stating that) the Vector Function ABI name
mangling scheme suffices to represent information 1. and 2.

 3. The directive is saying something more, as it lists `another_trait` in the
`construct` set, or might be listing an `extension` set with `vendor` specific
rules.

All this addition information doesn’t invalidate what is encoded in 1. And 2.  

This means that to represent `declare variant` information we don’t have to
worry about _extending_ the `vector-variant` attribute, but that we just need to
add additional attributes attached to the same `variant_foo` that describe the
additional context information expressed in the `declare variant`.

So, at the end, once we have full support for `declare variant`, the IR
attribute that we will associate to `foo` will look like:

```
. . . = call double @foo(double . . .) #0

attribute #0 = {vector-variant=“_ZGVwhatever_foo”, vendor-variant=“X”,
extension-variant=“Y”, another_trait_variant=“A,B,C”}

```

It will be then a matter of interpreting the data in attribute #0 to decide
whether or not it is appropriate/convenient to use `variant_foo` instead of
`foo` in whatever optimization is being performed.

As a last comment. Maybe the name `vector-variant` for the attribute is to close
to use of `variant` in the `declare variant` directive.

If that is something that have helped raising the idea that with my proposal I
was trying to use `vector-variant` to implement `declare variant`, I am happy to
change it to something that is specific to the `declare simd` context, something
like `declare-simd = “_ZGVn. . . , _ZGVs. . .”`. Lack of fantasy here, but I
must say that after thinking about the information it represent, I am kinda
incline in preferring it over `vector-variant`. This shouldn’t be a problem for
the VecCLone pass because the pass it not yet in master and therefore there is
no prior IR that uses `vector-variant` in production.
> To summarize: I will not oppose this effort but I would much rather see
> us working towards "declare variant”,
Hopefully this last message is a step in that direction.
> a stronger concept we will have to
> support in the near future anyway.
> 
> Thanks,
>  Johannes
You are very welcome, and thank you for forcing me thinking more about this
aspect!

Kind regards,

Francesco

On 06/04, Francesco Petrogalli wrote:
> 
> 
> > On Jun 4, 2019, at 12:23 PM, Doerfert, Johannes <jdoerfert at
anl.gov> wrote:
> > 
> > Hi Francesco,
> > 
> > On 06/03, Francesco Petrogalli wrote:
> >>> On Jun 3, 2019, at 1:43 PM, Andrea Bocci <andrea.bocci at
cern.ch> wrote:
> >>> as a candidate future user of the proposed extension, I think
I like the simplified proposal better than the original RFC.
> >>> 
> >>> The only part of the syntax that I would find not very much
user-friendly is having to mangle the isa/mask/vlen/type list by hand.
> >>> 
> >>> Would a more C-like syntax be feasible ?
> >> 
> >> The syntax I proposed allows user to expose concepts like
“linear”, “uniform” and “aligned" parameters, as described in the `declare
simd` directive of OpenMP, which is what x86 and aarch64 Vector Function ABIs
use to mangle the names.
> >> 
> >> I agree with you that having to manually specify the mangling is a
bit cumbersome, I am open to suggestions.
> >> 
> >> An alternative approach was to make the actual string inside the
attribute to accept anything a `declare variant` in a `simd` context could
accept, but that wold probably raise the same problems that `declare variant`
has raised.
> >> 
> >> In fact, on AArch64, a typical use of declare variant would be
more complicated than just specifying `simd` in the `context`, with `simdlen`
and `[not]inbranch`. It would require also to specify the `isa` trait from the
`device` set, and to use a `vendor` specific set to be able to list scalable
(i.e. vector-length agnostic) functions for SVE.
> >> 
> >> This would definitely look like implementing `declare variant`, as
the attribute would need to accept something like the following:
> >> 
> >> ```
> >> . . .
__attribute__(simd_variant(“vector_version”,“context={simd(simdlen(2),notinbranch”},device={isa(“simd")}))
> >> ```
> >> 
> >> Using the sub-string from the mangled name to me has the following
advantages:
> >> 
> >> 1. No need to deal with `declare variant`.
> >> 2. [...]
> >> 3. In terms of usability, there is no need to reinvent the wheel.
The
> >> `declare variant` of OpenMP 5.0 is perfectly suited for what we
are
> >> doing here, it is just much more than what it is needed for
exposing
> >> user-defined function to the vectorizer. So, on the long term, I
think
> >> we should definitely implement `declare variant`. Whatever
attribute
> >> system we came up with this RFC, it will be easy to map the `simd`
> >> bits of `declare variant` to it.
> > 
> > I'm unsure about this. While I agree that avoiding to deal with
"declare
> > variant" will make this all much easier, you seem to agree that
we will
> > eventually have to do that anyway.
> 
> Please notice that by “avoiding to deal with `declare variant`” doesn’t
mean that I am not defining the attribute with the OpenMP directive `declare
variant` in mind. It just means that I don’t want (for now, int his RFC),
discuss how to implement _full_ support for `declare variant` for the feature
discussed in this RFC.
> 
> > Adding something now that is a subset
> > of what we will need will put as in an awkward position.
> 
> Agree, and that’s why we need to craft it carefully to make sure we don’t
get to that point. Which is something that owe are definitely doing.
> 
> > We will have to
> > unify the existing in-house concept with the standardized OpenMP way
in
> > terms of semantics as well as implementation.
> 
> Yes.
> 
> > I somehow doubt that this
> > will make things easier, especially if we later discover that the
> > in-house solution is somehow incompatible or not actually a subset.
> > 
> 
> Agreed. The semantic of `declare variant` has more meanings than the
`declare simd` itself. But the specs state that the `simd(other-stuff)` trait
that can be passed to the `construct` set mirrors the semantic of the `declare
simd` directive.
> 
> “The simd trait can be further defined with properties that match the
clauses accepted by the declare simddirective with the same name and semantics.”
(Page 52, lines 9-10).
> 
> The Vector Function ABI mangling scheme represent all the information that
can be passed via a `declare  simd` directive (more than that, because it
encodes also the target vector extension in the <isa> token). I can
definitely speak for the AArch64 one, but it seems the same method has been used
for the x86 one (@Hideki, @Xinmin, can you confirm?).
> 
> Therefore, we can all agree that the attribute `vector-variant`, as it has
been proposed, can be used to represent `declare simd` and `declare variant`,
the latter when used with the `simd` trait of the `construct` set.
> 
> The question that you raise is: “can we extend the information carried by
`vector-variant` to represent all the semantic of `declare variant`?“
> 
> Even if the answer to such question is no, is it really what we want to do?
> 
> I am just thinking at a situation where the `declare variant` is used not
for a single trait like `simd`, but also for other contexts.
> 
> ```
> #pragma omp declare variant(variant_foo)
match(construct={simd(simdlen(2),notinbranch),
another_trait(with_other_clause)}, device= {isa(TARGET_ISA)}, ...)
> double foo(double);
> 
> Vector_double variant_foo(vector_double);
> ```
> 
> The pragma here is giving the following information:
> 
> 1. The function `variant_foo` is obtained by `foo` with the following
instance of `declare simd`:
> 
> #pragma omp declare simd simdlen(2) notinbranch
> double foo(double);
> 
> 2. The function `variant_foo` is written using the instructions of the
TARGET_ISA specified in the `isa` trait of the `device` set.
> 
> Again, (I will never get tired stating that) the Vector Function ABI name
mangling scheme suffices to represent information 1. and 2.
> 
>  3. The directive is saying something more, as it lists `another_trait` in
the `construct` set, or might be listing an `extension` set with `vendor`
specific rules.
> 
> All this addition information doesn’t invalidate what is encoded in 1. And
2.
> 
> This means that to represent `declare variant` information we don’t have to
worry about _extending_ the `vector-variant` attribute, but that we just need to
add additional attributes attached to the same `variant_foo` that describe the
additional context information expressed in the `declare variant`.
> 
> So, at the end, once we have full support for `declare variant`, the IR
attribute that we will associate to `foo` will look like:
> 
> ```
> . . . = call double @foo(double . . .) #0
> 
> attribute #0 = {vector-variant=“_ZGVwhatever_foo”, vendor-variant=“X”,
extension-variant=“Y”, another_trait_variant=“A,B,C”}
> 
> ```
Now the attributes are at the call site which make certain things
easier. I'm undecided though. I think given all use cases of `declare
variant` today, this is fine. Assuming we want to choose the variant at
some point after inlining, we might want to annotate the base functions
after all.

We should not have distinct attributes (regardless of the
location/encoding). Given that it is extensible, passes should not
accidentally miss context information. So we would have something like:

`omp-match={simd-variant="....", propertyX="..."}`

And `declare simd` is "just" a `declare variant match(simd=...).

We should think about all use cases of variant again to make sure this
would work for all of them.
> It will be then a matter of interpreting the data in attribute #0 to decide
whether or not it is appropriate/convenient to use `variant_foo` instead of
`foo` in whatever optimization is being performed.
> 
> As a last comment. Maybe the name `vector-variant` for the attribute is to
close to use of `variant` in the `declare variant` directive.
> 
> If that is something that have helped raising the idea that with my
proposal I was trying to use `vector-variant` to implement `declare variant`, I
am happy to change it to something that is specific to the `declare simd`
context, something like `declare-simd = “_ZGVn. . . , _ZGVs. . .”`. Lack of
fantasy here, but I must say that after thinking about the information it
represent, I am kinda incline in preferring it over `vector-variant`. This
shouldn’t be a problem for the VecCLone pass because the pass it not yet in
master and therefore there is no prior IR that uses `vector-variant` in
production.
We should not discuss names right now ;)

Cheers,
  Johannes
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20190606/226f1acb/attachment-0001.sig>