Page MenuHomePhabricator

[OpenMP][C++] Allow #pragma omp simd in constexpr functions
Needs ReviewPublic

Authored by jdoerfert on Oct 4 2022, 2:32 PM.

Details

Summary

There is no reason (obvious to me) that would prevent us from ignoring
the "simd" part during constexpr eval, assuming there are no clauses.
The potentially controversial part is the change from Equal to Encloses
when we do the context lookup.

Fixes https://github.com/llvm/llvm-project/issues/58092

Diff Detail

Event Timeline

jdoerfert created this revision.Oct 4 2022, 2:32 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 4 2022, 2:32 PM
jdoerfert requested review of this revision.Oct 4 2022, 2:32 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 4 2022, 2:32 PM
Herald added a subscriber: sstefan1. · View Herald Transcript

Adding a few more reviewers who have been poking around in constant expression evaluation lately.

clang/include/clang/Basic/DiagnosticSemaKinds.td
2722–2723

You should manually re-flow this to the usual 80-col limit.

clang/lib/AST/ExprConstant.cpp
8274–8279

This comment no longer matches the code. It also suggests that the way to address your issue is to capture the variable into the current frame -- did you explore that approach?

clang/lib/Sema/SemaDeclCXX.cpp
2175–2178

C++20 now has constexpr destructors, but there are also other kinds of special member functions too. It seems like the constraint really is "constant evaluation of a function with SIMD clauses is invalid" and so we might want to go with a more generic diagnostic instead of trying to distinguish between kinds of functions.

jdoerfert added inline comments.Oct 5 2022, 6:16 AM
clang/lib/AST/ExprConstant.cpp
8274–8279

The problem is that OpenMP introduces artificial capture statements. I am not sure if/how those can be "merged" into the parent context and in general that is not allowed (e.g., as we allow clauses on SIMD, like private). I feel the comment needs updating and I'll try that.

clang/lib/Sema/SemaDeclCXX.cpp
2175–2178

I copied this from below (see comment). I would assume the two have to stay in sync. All the differentiation of constructor does it to specify where it was found, no? So:
SIMD with clauses is not supported, found in XYZ.

2202

^ here

aaron.ballman added inline comments.Oct 7 2022, 8:54 AM
clang/lib/AST/ExprConstant.cpp
8274–8279

Is there a way to limit the change to only OpenMP captured variables so that we're not changing the semantics for all language modes?

clang/lib/Sema/SemaDeclCXX.cpp
2175–2178

Yeah, they should stay in sync; I don't insist on a change to the diagnostic as part of this review unless you feel like it.

jdoerfert added inline comments.Oct 7 2022, 10:49 AM
clang/lib/AST/ExprConstant.cpp
8274–8279

I could reasonably guard this to look for -fopenmp if we want to. The variables themselves are probably not easy to identify. I'll look at it again next week in the hope to make more sense of the intermediate frame and maybe avoid this to spill over to other models.