SjoerdMeijer added inline comments.
================
Comment at: clang/test/CodeGen/aarch64-sve-intrinsics/negative/acle_sve_ext.c:1
+// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -target-feature +sve
-fallow-half-arguments-and-returns -fsyntax-only -verify -D__ARM_FEATURE_SVE %s
+
----------------
sdesmalen wrote:
> SjoerdMeijer wrote:
> > sdesmalen wrote:
> > > SjoerdMeijer wrote:
> > > > sdesmalen wrote:
> > > > > SjoerdMeijer wrote:
> > > > > > 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)?
> > > > > 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.
> > > > 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?
> > > >
> > > >
> > > 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.
> > 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?
> 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.
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?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D76679/new/
https://reviews.llvm.org/D76679
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
