John, thank you for the review!
================ Comment at: include/clang/AST/ASTContext.h:89 @@ -85,2 +88,3 @@ + AlignIsRequired(AlignIsRequired) {} }; ---------------- rjmccall wrote: > Just make this a separate query on ASTContext. TypeInfo computation is > important to a lot of different clients, basically none of which care about > this. > > I don't think it's important to cache. Ok, will separate it. ================ Comment at: include/clang/Basic/TokenKinds.def:507 @@ +506,3 @@ +// OpenMP Type Traits +KEYWORD(simd_align , KEYALL) + ---------------- rjmccall wrote: > This is a very general name to give this, and putting it in the unreserved > namespace is problematic. I think that, given some time and coordination, we > could design a more general "what's the largest supported vector type for > this underlying type" query, and that would justify a name like this. But > the existing vector attributes always talk about, well, "vector" instead of > "simd"; and anyway, it's not obvious that the answer to that general query > would necessarily dictate the answer to this OpenMP-specific one. > > So let's design this as a more targeted feature around the needs of OpenMP. > In that context, "simd" is fine because it's exactly the name of the OpenMP > feature. How about "__builtin_openmp_simd_align"? Or even > "__builtin_openmp_required_simd_align"? Ok, turned it to __builtin_omp_required_simd_align. Actually, I thought about such kind of name, but it was too long, so I changed it to a shorter version. :) ================ Comment at: lib/AST/ItaniumMangle.cpp:3031 @@ -3023,3 +3030,3 @@ unsigned DiagID = Diags.getCustomDiagID(DiagnosticsEngine::Error, "cannot yet mangle vec_step expression"); Diags.Report(DiagID); ---------------- rjmccall wrote: > Copy-paste error. Fixed, thanks ================ Comment at: lib/Basic/Targets.cpp:2977 @@ -2975,1 +2976,3 @@ + + SimdDefaultAlign = (getABI() == "avx") ? 32 : 16; return true; ---------------- rjmccall wrote: > Aren't we saying that this is 64 for AVX512? There were no "avx512" defined when I prepared a patch. Will be added ================ Comment at: lib/CodeGen/CGExprScalar.cpp:2044 @@ -2040,1 +2043,3 @@ + CGF.getContext().getTypeInfo(E->getTypeOfArgument()).SimdDefaultAlign; + return Builder.getInt32(Alignment); } ---------------- rjmccall wrote: > This needs to produce a size_t, not an int32_t. You can use CGM::getSize. Thanks, fixed ================ Comment at: lib/Sema/SemaExpr.cpp:3522 @@ -3522,1 +3521,3 @@ + (TraitKind == UETT_SizeOf || TraitKind == UETT_AlignOf || + TraitKind == UETT_OpenMPDefaultSimdAlign)) { // sizeof(function)/alignof(function) is allowed as an extension. ---------------- rjmccall wrote: > This should be a hard error, not just an extension warning. Will be removed it and expressions won't be allowed as arguments ================ Comment at: lib/Sema/SemaExpr.cpp:3831 @@ -3827,1 +3830,3 @@ + isInvalid = + CheckUnaryExprOrTypeTraitOperand(E, UETT_OpenMPDefaultSimdAlign); } else if (E->refersToBitField()) { // C99 6.5.3.4p1. ---------------- rjmccall wrote: > Hmm. Thinking more about it, using this trait with an expression instead of > a type seems pretty misleading. Let's make this a hard error for now. We > can continue to use the UETT parsing logic and AST representation. Ok http://reviews.llvm.org/D10597 EMAIL PREFERENCES http://reviews.llvm.org/settings/panel/emailpreferences/ _______________________________________________ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits