sdesmalen marked an inline comment as done. sdesmalen 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 + ---------------- 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. 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
