[OPENMP] Update the diagnosis message for canonical loop form

[cchen]

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.

[lebedev.ri]

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

[lebedev.ri]

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

[ABataev]

This comment was removed by ABataev.

[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

[cchen]

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

[ABataev]

In D66559#1641827, @cchen wrote:

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]

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

[lebedev.ri]

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

[ABataev]

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–9032

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]

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.

[ABataev]

In D66559#1649775, @cchen wrote:

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]

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

[cchen]

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]

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

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

[ABataev]

In D66559#1651756, @cchen wrote:

"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

5443

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

5478

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

[cchen]

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

[ABataev]

LG, thanks for the fix!

[cchen]

Refactor for only load the OpenMP version number once

[cchen]

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]

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]

Fix the code that rely on implicit conversion.

[ABataev]

LG

[cchen]

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

[ABataev]

In D66559#1663887, @cchen wrote:

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

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

[cchen]

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.

[ABataev]

In D66559#1665289, @cchen wrote:

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.

clang/include/clang/Basic/DiagnosticSemaKinds.td

 def err_omp_local_var_in_threadprivate_init : Error<
   "variable with local storage in initial value of threadprivate variable">;
 def err_omp_loop_not_canonical_init : Error<
   "initialization clause of OpenMP for loop is not in canonical form "
   "('var = init' or 'T var = init')">;
 def ext_omp_loop_not_canonical_init : ExtWarn<
   "initialization clause of OpenMP for loop is not in canonical form "
   "('var = init' or 'T var = init')">, InGroup<OpenMPLoopForm>;
-def err_omp_loop_not_canonical_cond : Error<
+def err_omp_before_50_loop_not_canonical_cond : Error<
   "condition of OpenMP for loop must be a relational comparison "
-  "('<', '<=', '>', or '>=') of loop variable %0">;
+  "('<', '<=', '>', or '>=') of loop variable %0">;
+def err_omp_after_50_loop_not_canonical_cond : Error<
+  "condition of OpenMP for loop must be a relational comparison "
+  "('<', '<=', '>', '>=', or '!=') of loop variable %0">;
 def err_omp_loop_not_canonical_incr : Error<
   "increment clause of OpenMP for loop must perform simple addition "
   "or subtraction on loop variable %0">;
 def err_omp_loop_variable_type : Error<
   "variable must be of integer or %select{pointer|random access iterator}0 type">;
 def err_omp_loop_incr_not_compatible : Error<
   "increment expression must cause %0 to %select{decrease|increase}1 "
   "on each iteration of OpenMP for loop">;

clang/lib/Sema/SemaOpenMP.cpp

if (const auto *ME = dyn_cast_or_null<MemberExpr>(E))
     if (ME->isArrow() && isa<CXXThisExpr>(ME->getBase()->IgnoreParenImpCasts()))
       return getCanonicalDecl(ME->getMemberDecl());
   return nullptr;
 }
 
 bool OpenMPIterationSpaceChecker::checkAndSetCond(Expr *S) {
   // Check test-expr for canonical form, save upper-bound UB, flags for
   // less/greater and for strict/non-strict comparison.
-  // OpenMP [2.6] Canonical loop form. Test-expr may be one of the following:
+  // OpenMP [2.9] Canonical loop form. Test-expr may be one of the following:
   //   var relational-op b
   //   b relational-op var
   //
   if (!S) {
-    SemaRef.Diag(DefaultLoc, diag::err_omp_loop_not_canonical_cond) << LCDecl;
+    if (SemaRef.getLangOpts().OpenMP < 50)
+      SemaRef.Diag(DefaultLoc, diag::err_omp_before_50_loop_not_canonical_cond) << LCDecl;
+    else
+      SemaRef.Diag(DefaultLoc, diag::err_omp_after_50_loop_not_canonical_cond) << LCDecl;
     return true;
   }
   Condition = S;
   S = getExprAsWritten(S);
   SourceLocation CondLoc = S->getBeginLoc();
   if (auto *BO = dyn_cast<BinaryOperator>(S)) {
     if (BO->isRelationalOp()) {
       if (getInitLCDecl(BO->getLHS()) == LCDecl)
         return setUB(BO->getRHS(),
                      (BO->getOpcode() == BO_LT || BO->getOpcode() == BO_LE),
                      (BO->getOpcode() == BO_LT || BO->getOpcode() == BO_GT),
                      BO->getSourceRange(), BO->getOperatorLoc());
       if (getInitLCDecl(BO->getRHS()) == LCDecl)
         return setUB(BO->getLHS(),
                      (BO->getOpcode() == BO_GT || BO->getOpcode() == BO_GE),
                      (BO->getOpcode() == BO_LT || BO->getOpcode() == BO_GT),
                      BO->getSourceRange(), BO->getOperatorLoc());
     } else if (BO->getOpcode() == BO_NE)
         return setUB(getInitLCDecl(BO->getLHS()) == LCDecl ?
                        BO->getRHS() : BO->getLHS(),
                      /*LessOp=*/llvm::None,
                      /*StrictOp=*/true,
                      BO->getSourceRange(), BO->getOperatorLoc());
   } else if (auto *CE = dyn_cast<CXXOperatorCallExpr>(S)) {
     if (CE->getNumArgs() == 2) {
       auto Op = CE->getOperator();
       switch (Op) {
if (CE->getNumArgs() == 2) {
                      CE->getSourceRange(),
                      CE->getOperatorLoc());
         break;
       default:
         break;
       }
     }
   }
   if (dependent() || SemaRef.CurContext->isDependentContext())
     return false;
-  SemaRef.Diag(CondLoc, diag::err_omp_loop_not_canonical_cond)
-      << S->getSourceRange() << LCDecl;
+  if (SemaRef.getLangOpts().OpenMP < 50)
+    SemaRef.Diag(CondLoc, diag::err_omp_before_50_loop_not_canonical_cond)
+        << S->getSourceRange() << LCDecl;
+  else
+    SemaRef.Diag(CondLoc, diag::err_omp_after_50_loop_not_canonical_cond)
+        << S->getSourceRange() << LCDecl;
   return true;
 }
 
 bool OpenMPIterationSpaceChecker::checkAndSetIncRHS(Expr *RHS) {
   // RHS of canonical loop form increment can be:
   //   var + incr
   //   incr + var
   //   var - incr