SjoerdMeijer added inline comments.
================ Comment at: clang/lib/Basic/Targets/ARM.cpp:958 case llvm::ARM::ArchKind::ARMV8_6A: + case llvm::ARM::ArchKind::ARMV8_7A: case llvm::ARM::ArchKind::ARMV8_8A: ---------------- tyb0807 wrote: > SjoerdMeijer wrote: > > tyb0807 wrote: > > > SjoerdMeijer wrote: > > > > I see tests for the crypto stuff, but is there or do we need a test for > > > > whatever `getTargetDefinesARMV83A` is setting? > > > I'm not sure that we need a test for this, as none of the other > > > architectures really have this. What do you think @SjoerdMeijer > > > @vhscampos ? > > I am expecting tests for the ACLE macros that this patch defines for v8.7 > > to be added to `clang/test/Preprocessor/arm-target-features.c`. > In that case, for consistency, I think we should also add tests for ACLE > macros for other versions (v8.4/5/6/8) as well. And for AArch64 too. It > sounds a bit out of scope, but it ensures consistency IMHO The subject says > [ARM][AArch64] Add missing v8.x checks so looks perfect in scope to me. :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116153/new/ https://reviews.llvm.org/D116153 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
