Page MenuHomePhabricator

[OPENMP] Update the diagnosis message for canonical loop form
ClosedPublic

Authored by cchen on Aug 21 2019, 2:50 PM.

Details

Summary

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.

Event Timeline

cchen created this revision.Aug 21 2019, 2:50 PM
Herald added a project: Restricted Project. · View Herald Transcript

Shouldn't this be OpenMP-version-dependent?
!= is only allowed in 4.5+

Or more specifically, why != is now silently accepted in 4.0 mode?
https://godbolt.org/z/M6iL4H

cchen added a project: Restricted Project.Aug 21 2019, 3:32 PM
This comment was removed by ABataev.

!= 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

lebedev.ri requested changes to this revision.Aug 22 2019, 6:50 AM
This revision now requires changes to proceed.Aug 22 2019, 6:50 AM
cchen added a comment.Aug 22 2019, 2:13 PM

Is there any way that I can find the OpenMP version in Clang? I couldn't find any example in source code. Thanks.

Is there any way that I can find the OpenMP version in Clang? I couldn't find any example in source code. Thanks.

LangOpts.OpenMP has the version of OpenMP. For OpenMP 5.0 this value is set to 50.

cchen updated this revision to Diff 217710.Aug 28 2019, 1:19 PM

Update the diagnosis message for canonical loop form based on OpenMP version

!= is still not being diagnosed in pre-5.0 mode though?

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
9027–9030

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"

clang/lib/Sema/SemaOpenMP.cpp
5422

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;
cchen added a comment.Aug 28 2019, 1:26 PM

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.

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.

cchen added a comment.Aug 28 2019, 1:32 PM

@ABataev thanks so much for your instruction, I'll look into that.

cchen updated this revision to Diff 217978.Aug 29 2019, 2:44 PM

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.

cchen added a comment.Aug 29 2019, 2:48 PM

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

lebedev.ri resigned from this revision.Aug 29 2019, 2:56 PM

Looks about right now, thanks for unbreaking it.

clang/lib/Sema/SemaOpenMP.cpp
5419–5420

It may be much cleaner to

bool IneqCondIsCanonical = SemaRef.getLangOpts().OpenMP >= 50;

and use that

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

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
5439–5440

Just make if(OpenMP >= 50 && opcode == BO_NE) and remove else branch

5471

else branch is not required here, the control will go to the default path with diagnostics.

cchen updated this revision to Diff 218116.Aug 30 2019, 9:16 AM

Refactor the control flow based on the feedback, fix a failing codegen test due
to OpenMP version.

ABataev accepted this revision.Aug 30 2019, 9:22 AM

LG, thanks for the fix!

This revision is now accepted and ready to land.Aug 30 2019, 9:22 AM
cchen updated this revision to Diff 218118.Aug 30 2019, 9:27 AM
cchen marked 4 inline comments as done.

Refactor for only load the OpenMP version number once

cchen marked an inline comment as done.Aug 30 2019, 10:20 AM

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.

ABataev added inline comments.Aug 30 2019, 10:28 AM
clang/lib/Sema/SemaOpenMP.cpp
5422

I would not suggest to rely on the conversion of bool values to integer, better to use IneqCondIsCanonical ? 0 : 1

cchen updated this revision to Diff 218146.Aug 30 2019, 12:01 PM

Fix the code that rely on implicit conversion.

cchen marked an inline comment as done.Sep 9 2019, 2:58 PM

@ABataev, could you mark it as ready to land, please. Thanks.

@ABataev, could you mark it as ready to land, please. Thanks.

I accepted it already, don't know what else I can do.

cchen added a comment.Sep 10 2019, 2:35 PM

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.

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.

Sure, will do this tomorrow.

This revision was automatically updated to reflect the committed changes.