[PATCH] D76679: [SveEmitter] Add more immediate operand checks.

[Sjoerd Meijer via Phabricator via cfe-commits]

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:
> > > > 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? 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76679/new/

https://reviews.llvm.org/D76679