The previous patch (https://reviews.llvm.org/D54441) support the
relational-op != very well for openmp canonical loop form, however,
it didn't update the diagnosis message. So this patch is simply
update the diagnosis message by adding !=, update the test
related to it, and update the section number for canonical loop
form for OpenMP 5.0 in comment.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Or more specifically, why != is now silently accepted in 4.0 mode?
https://godbolt.org/z/M6iL4H
!= form is part of OpenMP 5.0. It would be good to allow this form for OpenMP 5.0 only and emit this new error message only for OpenMP 5.0
Is there any way that I can find the OpenMP version in Clang? I couldn't find any example in source code. Thanks.
What about the check for !=? it would be good to allow it only for OpenMP 5.0.
Also, add/update the tests.
clang/include/clang/Basic/DiagnosticSemaKinds.td | ||
---|---|---|
9095–9098 | You can merge these 2 messages into one using something like | |
clang/lib/Sema/SemaOpenMP.cpp | ||
5426 | If you merge 2 messages into one, you can diagnose it this way: SemaRef.Diag(DefaultLoc, diag::err_omp_loop_not_canonical_cond) << (SemaRef.getLangOpts().OpenMP <= 45 ? 0 : 1) << LCDecl; |
Haven't updated the lit test for OpenMP 5.0 yet since I'm not able to check OPENMP version by using _OPENMP with preprocessor condition checking.
You don't need _OPENMP macro for this. There a lot of tests in the OpenMP directory for OpenMP 5.0 already, look for -fopenmp-version=50 option in the tests.
Update lit test for diagnose loop canonical form message for OpenMP 5.0 and
pre-5.0. Only allow fopenmp-version=50 to use "!=" as check condition in loop
canonical form.
"parallel_for_codegen" cannot pass now since it has two test cases for "!=" and it's hard to just add "fopenmp-version=50" since it will break lots of other test cases that have different codegen for OpenMP 5.0.
I can remove the "!=" case from "parallel_for_codegen" and add a new file such as "omp50_parallel_for_codegen" to test it but I doubt it's the right way to do it.
Looks about right now, thanks for unbreaking it.
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
5422–5424 | It may be much cleaner to bool IneqCondIsCanonical = SemaRef.getLangOpts().OpenMP >= 50; and use that |
Add a new RUN directive(s) for openmp 5.0 and add a definition specific for OpenMP 5.0 run. In the code use #ifdef...endif to check that version is OpenMP 5.0 and the test with!= must be under this guard.
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
5443–5444 | Just make if(OpenMP >= 50 && opcode == BO_NE) and remove else branch | |
5474 | else branch is not required here, the control will go to the default path with diagnostics. |
Refactor the control flow based on the feedback, fix a failing codegen test due
to OpenMP version.
I'm not able to arc land my patch since I updated the patch for a minor refactor and the state is "not approve" now. Do I wait for the approval for the new patch so that I can arc land or I don't need to do arc land myself?
Thanks.
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
5426 | I would not suggest to rely on the conversion of bool values to integer, better to use IneqCondIsCanonical ? 0 : 1 |
Oh, I was thinking that I could use "arc land" to commit my patch, but just now realize that I don't have the commit privileges. Would you please commit for me? Thanks.
You can merge these 2 messages into one using something like
"condition of OpenMP for loop must be a relational comparison ('<', '<=', '>', %select{or '>='|'>=', or '!='}0) of loop variable %1"