Hi Vladimir, > On 24 Jul 2024, at 11:41, Vladimir Miloserdov <vladimir.miloser...@arm.com> > wrote: > > External email: Use caution opening links or attachments > > > Hi Kyrill, > > Thanks for your prompt review again! > >> It looks like __ARM_FEATURE_LUT should guard the Advanced SIMD intrinsics >> for LUTI at least. >> Therefore we should add this macro definition only once those are >> implemented as well. So I’d remove this hunk. > I'm working on adding those Advanced SIMD intrinsics now. The flag should be > added by either SVE or AdvSIMD LUTI intrinsics patch from my understanding. I > think this one is first to go in, so should we leave it here?
As the macro gives the user information that both SVE and AdvSIMD intrinsics are available, its definition should only go in once both AdvSIMD and SVE intrinsics are implemented. So either this patch goes in after the AdvSIMD one, or the macro definition goes into the AdvSIMD patch after this patch goes in. Thanks, Kyrill > >> Since this patch depends on Andrew’s feature flags rework shouldn’t this >> hunk look like: >> AARCH64_HAVE_ISA (LUT) >> Instead? > Yes, looks like I was using an older revision of Andrew's work. I'll wait for > it to be merged and update the patch then. > >> +# Return 1 if this is an AArch64 target supporting LUT (Lookup table) >> I don’t see this effective target check used anywhere, do we need to add it? >> I guess I don’t mind it for future use, but just checking that it was >> deliberate. > It is deliberate - I thought it may be useful to have for future use. > > BR, > - Vladimir Miloserdov
