This is an archive of the discontinued LLVM Phabricator instance.

[SveEmitter] Add more immediate operand checks.
ClosedPublic

Authored by sdesmalen on Mar 24 2020, 2:00 AM.

Details

Summary

This patch adds a number of intrinsics that take immediates with
varying ranges based on the element size one of the operands.

svext:   immediate ranging 0 to (2048/sizeinbits(elt) - 1)
svasrd:  immediate ranging 1..sizeinbits(elt)
svqshlu: immediate ranging 1..sizeinbits(elt)/2
ftmad:   immediate ranging 0..(sizeinbits(elt) - 1)

Diff Detail

Event Timeline

sdesmalen created this revision.Mar 24 2020, 2:00 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: tschuett. · View Herald Transcript
SjoerdMeijer added inline comments.Mar 24 2020, 1:11 PM
clang/test/CodeGen/aarch64-sve-intrinsics/negative/acle_sve_ext.c
1

Just curious about the -fallow-half-arguments-and-returns, do you need that here?

And if not here, why do you need it elsewhere (looks enabled on all tests)?

sdesmalen marked an inline comment as done.Apr 1 2020, 3:54 AM

(sorry, I wrote the comments earlier but forgot to click 'submit' :) )

clang/test/CodeGen/aarch64-sve-intrinsics/negative/acle_sve_ext.c
1

It's not needed for this test, but we've generated most of our tests from the ACLE spec and the tests that use a scalar float16_t (== __fp16) will need this, such as the ACLE intrinsic:

svfloat16_t svadd_m(svbool_t, svfloat16_t, float16_t);

If you feel strongly about it, I could remove it from the other RUN lines.

clang/test/CodeGen/aarch64-sve-intrinsics/negative/acle_sve_ext.c
1

Well, I think this is my surprise then. Thinking out loud: we're talking SVE here, which always implies FP16. That's why I am surprised that we bother with a storage-type only type. Looking at the SVE ACLE I indeed see:

float16_t equivalent to __fp16

where I was probably expecting:

float16_t equivalent to _Float16

and with that everything would be sorted I guess, then we also don't need the hack^W workaround that is -fallow-half-arguments-and-returns. But maybe there is a good reason to use/choose __fp16 that I don't see here. Probably worth a quick question for the ARM SVE ACLE, would you mind quickly checking?

sdesmalen marked an inline comment as done.Apr 1 2020, 7:14 AM
sdesmalen added a subscriber: rsandifo-arm.
sdesmalen added inline comments.
clang/test/CodeGen/aarch64-sve-intrinsics/negative/acle_sve_ext.c
1

As just checked with @rsandifo-arm, the reason is that the definition of float16_t has to be compatible with arm_neon.h, which uses __fp16 for both Clang and GCC.

SjoerdMeijer added inline comments.Apr 1 2020, 8:00 AM
clang/test/CodeGen/aarch64-sve-intrinsics/negative/acle_sve_ext.c
1

I was suspecting it was compatability reasons, but perhaps not with arm_neon.h. So what exactly does it mean to be compatible with arm_neon.h? I mean, put simply and naively, if you target SVE, you include arm_sve.h, and go from there. How does that interact with arm_neon.h and why can float16_t not be a proper half type?

sdesmalen marked an inline comment as done.Apr 1 2020, 8:57 AM
sdesmalen added inline comments.
clang/test/CodeGen/aarch64-sve-intrinsics/negative/acle_sve_ext.c
1

If you target SVE, you can still use Neon instructions, so it's still possible to include arm_neon.h as well. If those have differing definitions of float16_t, that may give trouble when using builtins from both the Neon and SVE header files.

SjoerdMeijer added inline comments.Apr 1 2020, 9:46 AM
clang/test/CodeGen/aarch64-sve-intrinsics/negative/acle_sve_ext.c
1

ok, thank, got it. So we are supporting this case:

#include <arm_neon.h>
#include <arm_svh.h>
void foo () {
   neon_intrinsic();
   sve_intrinsic();
}

Well, I find this very unfortunate, because it could have been so beautiful, and now we're still have this storage-only type while even the ACLE discourages its use. The use of -fallow-half-arguments-and-returns is just a minor annoyance, but point is it shouldn't have been necessary.

Now I am wondering why the ARM SVE ACLE is using float16_t, and not just _Float16. Do you have any insights in that too perhaps?

Just to clarify my last sentence:

Now I am wondering why the ARM SVE ACLE is using float16_t, and not just _Float16. Do you have any insights in that too perhaps?

What I meant to say is why SVE intrinsics are not using _Float16?

SjoerdMeijer accepted this revision.Apr 2 2020, 8:53 AM

I think the float16 discussion is an interesting one, but doesn't necessarily need to be done here. I am asking some questions offline, but if we ever come to a different opinion on it, then we can follow up so it's somewhat orthogonal to this change, and so this looks fine to me.

This revision is now accepted and ready to land.Apr 2 2020, 8:53 AM

I think the float16 discussion is an interesting one, but doesn't necessarily need to be done here. I am asking some questions offline, but if we ever come to a different opinion on it, then we can follow up so it's somewhat orthogonal to this change, and so this looks fine to me.

Thanks for reviewing the patch Sjoerd!

This revision was automatically updated to reflect the committed changes.