This is an archive of the discontinued LLVM Phabricator instance.

[OPENMP] Introduced type trait "__builtin_omp_required_simd_align" for default simd alignment.
ClosedPublic

Authored by ABataev on Jun 22 2015, 2:11 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

ABataev updated this revision to Diff 28101.Jun 22 2015, 2:11 AM
ABataev retitled this revision from to [OPENMP] Introduced type trait "simd_align" for default simd alignment..
ABataev updated this object.
ABataev edited the test plan for this revision. (Show Details)
ABataev added a reviewer: rjmccall.
ABataev added a subscriber: Unknown Object (MLST).
rjmccall added inline comments.Jun 22 2015, 3:26 PM
include/clang/AST/ASTContext.h
89 ↗(On Diff #28101)

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.

include/clang/Basic/TokenKinds.def
507 ↗(On Diff #28101)

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

lib/AST/ItaniumMangle.cpp
3031 ↗(On Diff #28101)

Copy-paste error.

lib/Basic/Targets.cpp
2977 ↗(On Diff #28101)

Aren't we saying that this is 64 for AVX512?

lib/CodeGen/CGExprScalar.cpp
2044 ↗(On Diff #28101)

This needs to produce a size_t, not an int32_t. You can use CGM::getSize.

lib/Sema/SemaExpr.cpp
3522 ↗(On Diff #28101)

This should be a hard error, not just an extension warning.

3831 ↗(On Diff #28101)

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.

John, thank you for the review!

include/clang/AST/ASTContext.h
89 ↗(On Diff #28101)

Ok, will separate it.

include/clang/Basic/TokenKinds.def
507 ↗(On Diff #28101)

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. :)

lib/AST/ItaniumMangle.cpp
3031 ↗(On Diff #28101)

Fixed, thanks

lib/Basic/Targets.cpp
2977 ↗(On Diff #28101)

There were no "avx512" defined when I prepared a patch. Will be added

lib/CodeGen/CGExprScalar.cpp
2044 ↗(On Diff #28101)

Thanks, fixed

lib/Sema/SemaExpr.cpp
3522 ↗(On Diff #28101)

Will be removed it and expressions won't be allowed as arguments

3831 ↗(On Diff #28101)

Ok

ABataev updated this revision to Diff 28205.Jun 23 2015, 12:44 AM

Fixed after review.

rjmccall edited edge metadata.Jun 25 2015, 3:43 PM

"omp" instead of "openmp" makes a lot sense, since that's what's used in the pragma.

include/clang/Basic/DiagnosticSemaKinds.td
4688 ↗(On Diff #28205)

You lost the 0 here.

include/clang/Basic/TargetInfo.h
397 ↗(On Diff #28205)

Please embellish this comment to make it clear that it's sometimes type-specific.

It would be great if we could move the type-specific logic to TargetInfo, but I don't know of a way to do that without either (1) making a lot of assumptions about what kinds of types are interesting to the target or (2) creating a cyclic dependency between lib/Basic and lib/AST. Ugh. We can stick with what we have right now.

lib/AST/ASTContext.cpp
1791 ↗(On Diff #28205)

I think this query should probably apply to the underlying type, not to the pointer type. That is, if I want to know the expected alignment of an array of doubles, I should use

__builtin_omp_required_simd_align(double)

not

__builtin_omp_required_simd_align(double*)

Unless PPC64's rule really is about arrays of pointers to double?

lib/CodeGen/CGExprScalar.cpp
2045 ↗(On Diff #28205)

I assume the type of the argument here is always a pointer, which means you'll need to drill down to the element type.

lib/Parse/ParseExpr.cpp
1046 ↗(On Diff #28205)

These comments are supposed to be grammar specifications; whoever implemented vec_step apparently missed that.

What you want is:

unary-expression: '__builtin_omp_required_simd_align' '(' type-name ')'

(since we don't actually allow the expression form semantically)

ABataev updated this revision to Diff 28541.Jun 26 2015, 12:17 AM
ABataev updated this object.
ABataev edited edge metadata.

Updated after review

ABataev retitled this revision from [OPENMP] Introduced type trait "simd_align" for default simd alignment. to [OPENMP] Introduced type trait "__builtin_omp_required_simd_align" for default simd alignment..Jun 26 2015, 12:18 AM

One more tweak, and then LGTM.

include/clang/Basic/TypeTraits.h
96 ↗(On Diff #28541)

Please rename this to follow the actual name of the trait, i.e. UETT_OpenMPRequiredSimdAlign.

This revision was automatically updated to reflect the committed changes.