tra accepted this revision. tra added a comment. This revision is now accepted and ready to land.
LGTM with a nit. ================ Comment at: clang/lib/Sema/SemaType.cpp:8168 + IsTargetCUDAAndHostARM = + !AuxTI || AuxTI->getTriple().isAArch64() || AuxTI->getTriple().isARM(); + } ---------------- alexander-shaposhnikov wrote: > tra wrote: > > Should it be `AuxTI && (AuxTI->getTriple().isAArch64() || > > AuxTI->getTriple().isARM();)` ? > > > > I don't think we want IsTargetCUDAAndHostARM to be set to true if there's > > no auxTargetInfo (e.g. during any C++ compilation, regardless of the > > target). > we get here only if S.getLangOpts().CUDAIsDevice is true, so not for an > arbitrary c++ compilation, > iirc AuxTI was null for some tests, but I'm happy to double check, > AuxTI && ... looks better to me too. I'd still prefer to have `()` around `AuxTI->getTriple().isAArch64() || AuxTI->getTriple().isARM()`. ================ Comment at: clang/test/SemaCUDA/neon-attrs.cu:2 +// RUN: %clang_cc1 -triple arm64-linux-gnu -target-feature +neon -x cuda -fsyntax-only -DNO_DIAG -verify %s +// RUN: %clang_cc1 -triple arm64-linux-gnu -target-feature -neon -x cuda -fsyntax-only -verify %s + ---------------- alexander-shaposhnikov wrote: > tra wrote: > > You should also pass `-aux-triple nvptx64...`. > > > > This also needs more test cases. This only tests host-side CUDA compilation. > > We also need: > > ``` > > // GPU-side compilation on ARM (no errors expected) > > // RUN: %clang_cc1 -aux-triple arm64-linux-gnu -triple nvptx64 > > -fcuda-is-device -x cuda -fsyntax-only -DNO_DIAG -verify %s > > // Regular C++ compilation on x86 and ARM without neon (should produce > > diagnostics) > > // RUN: %clang_cc1 -triple x86.... -x c++ -fsyntax-only -verify %s > > // RUN: %clang_cc1 -triple arm64... -x c++ -target-feature -neon > > -fsyntax-only -verify %s > > // C++ on ARM w/ neon (no diagnostics) > > // RUN: %clang_cc1 -triple arm64... -x c++ -target-feature +neon > > -fsyntax-only -DNO_DIAG -verify %s > > ``` > regular C++ compilation is covered by other in-tree tests, do we really need > it here ? If it's already covered (for x86, too?), then you can skip c++ tests. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D152403/new/ https://reviews.llvm.org/D152403 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
