This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP 5.2] Initial parsing and semantic analysis support for 'step' modifier on 'linear' clause
ClosedPublic

Authored by mdfazlay on Sep 25 2023, 1:40 PM.

Details

Summary

According to OpenMP 5.2 Specs, the following are valid syntaxes:

#pragma omp ... linear(var1, var2: val, step(2))
#pragma omp ... linear(var1, var2: step(-2), val)
#pragma omp ... linear(var1, var2: step(-2))
#pragma omp ... linear(var1: -2)

Reference:

(1) OpenMP 5.2 Specification - Section 5.4.6

Diff Detail

Event Timeline

mdfazlay created this revision.Sep 25 2023, 1:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 25 2023, 1:40 PM
mdfazlay requested review of this revision.Sep 25 2023, 1:40 PM
ABataev added inline comments.Sep 26 2023, 9:15 AM
clang/lib/Parse/ParseOpenMP.cpp
4795–4797

This is bad approach. Parser shall jut scan the token and translate it to OpenMP related enumeric, actual checking should be in Sema

mdfazlay updated this revision to Diff 557719.Oct 16 2023, 5:05 PM
mdfazlay added inline comments.
clang/lib/Parse/ParseOpenMP.cpp
4795–4797

I have updated this revision. Please take a look and let me know your feedback :)

ABataev added inline comments.Oct 17 2023, 10:16 AM
clang/include/clang/Basic/DiagnosticParseKinds.td
1389–1390

It must be in Sema

clang/lib/Parse/ParseOpenMP.cpp
4404
  1. parseStepSize
  2. Add a comment describing this function
  3. Make it return bool instead of setting StepFound
4416

I arther doubt you need a new warning message here, better to rely on the existing functionality for extra tokens.

4797

Ussually, it tries to translate token to OMPC_ kind rather than direct string comparison

4829

This check must be in Sema

4836–4844

Use else if

4858–4860

Parser should not do this, all this analysis must be performed in Sema

mdfazlay updated this revision to Diff 557800.Oct 19 2023, 10:27 PM
mdfazlay marked an inline comment as done.
mdfazlay added inline comments.Oct 19 2023, 10:43 PM
clang/include/clang/Basic/DiagnosticParseKinds.td
1389–1390

Done.

clang/lib/Parse/ParseOpenMP.cpp
4404

Done.

4416

Reused an existing warning message.

4797

I tried to add it here, but it would change the functionality of Sema::CheckOpenMPLinearModifier as the semantics of linear kind (val, uval, ref) and step modifier are different. Saving the step modifier location would help us to give the necessary diagnosis in Sema.

4829

Done.

4858–4860

This is part of the existing code for OMPC_aligned and OMPC_linear (with OMP <= 51) and I haven't changed it. We could try to modify it in a follow-up PR as it would involve changing the existing implementation of OMPAlinedClause and OMPLinearClause.

ABataev added inline comments.Oct 20 2023, 3:21 AM
clang/include/clang/Basic/DiagnosticParseKinds.td
1351–1353

Rather doubt this should be in ExtraTokens group

clang/include/clang/Sema/Sema.h
12310

Transform comment into doxygen comment

clang/lib/Parse/ParseOpenMP.cpp
4416–4420

What I meant, that you need to diagnose absence of ')' and make all other prser parts do their work, no need to skip to the end and emit a warning here

4423

No need for else after return

4424–4425

No, that's a bad decision to skip everything here

4797

Need to implement special handling for this in Sema::CheckOpenMPLinearModifier, but still rely on OMPC_ kind

mdfazlay updated this revision to Diff 557846.Oct 23 2023, 11:05 AM
mdfazlay marked 2 inline comments as done.
mdfazlay added inline comments.
clang/include/clang/Basic/DiagnosticParseKinds.td
1351–1353

Removed InGroup<ExtraTokens>.

clang/lib/Parse/ParseOpenMP.cpp
4416–4420

Done. Please take a look.

4423

Removed.

4424–4425

Removed.

4797

Done. Please take a look.

ABataev added inline comments.Oct 24 2023, 9:51 AM
clang/include/clang/Basic/DiagnosticParseKinds.td
1351–1353

No, each warning must belong to some group, just ExtraTokens is not the right one. Use SourceUsesOpenMP instead. Also, I think this should be an error, not a warning

clang/lib/Parse/ParseOpenMP.cpp
4812–4815
if (!T.consumeOpen())
  Diag(StepModifierLoc, diag::err_expected_lparen_after) << "step";
4818–4819

Avoid assingment in if

4823–4827

I think just T.consumeClose(); should be enough

mdfazlay updated this revision to Diff 557869.Oct 24 2023, 12:33 PM
mdfazlay marked 3 inline comments as done.
mdfazlay added inline comments.
clang/lib/Parse/ParseOpenMP.cpp
4812–4815

Done. It turns out that it has to be if (T.consumeOpen())

4818–4819

Done.

4823–4827

Done.

mdfazlay marked an inline comment as done.Oct 24 2023, 12:35 PM
mdfazlay added inline comments.
clang/include/clang/Basic/DiagnosticParseKinds.td
1351–1353

Done. Please take a look.

ABataev accepted this revision.Oct 24 2023, 12:52 PM

LG with a nit

clang/lib/Parse/ParseOpenMP.cpp
4785–4786

Adjust this comment, please

This revision is now accepted and ready to land.Oct 24 2023, 12:52 PM
mdfazlay updated this revision to Diff 557870.Oct 24 2023, 1:22 PM
mdfazlay marked an inline comment as done.
mdfazlay added inline comments.
clang/lib/Parse/ParseOpenMP.cpp
4785–4786

Done. Please take a look.

ABataev accepted this revision.Oct 24 2023, 1:24 PM

Still LGTM :)

This revision was landed with ongoing or failed builds.Oct 24 2023, 3:30 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 24 2023, 3:30 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
jrtc27 added a subscriber: jrtc27.Oct 25 2023, 12:02 PM
jrtc27 added inline comments.
clang/include/clang/Basic/DiagnosticParseKinds.td
1352

This trailing whitespace broke CI's formatting check

jrtc27 added inline comments.Oct 25 2023, 12:04 PM
clang/include/clang/Basic/DiagnosticParseKinds.td
1352

(I've gone and fixed it in 0ab694754e3722f7edbd8b3ad23ac0b312515d3b)