This is an archive of the discontinued LLVM Phabricator instance.

[OPENMP51]Initial parsing/sema for adjust_args clause for 'declare variant'
ClosedPublic

Authored by mikerice on Apr 5 2021, 4:09 PM.

Details

Summary

Adds initial parsing and sema for the 'adjust_args' clause.

Note that an AST clause is not created as it instead adds its expressions to the OMPDeclareVariantAttr.

Diff Detail

Event Timeline

mikerice created this revision.Apr 5 2021, 4:09 PM
mikerice requested review of this revision.Apr 5 2021, 4:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 5 2021, 4:09 PM
aaron.ballman added inline comments.Apr 8 2021, 9:22 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
10626
clang/lib/Parse/ParseOpenMP.cpp
1447–1452
1453

Should this return IsError?

clang/lib/Sema/SemaOpenMP.cpp
7089–7094
7109

Do we have to worry about things like MemberExpr as well?

mikerice updated this revision to Diff 336226.Apr 8 2021, 2:10 PM
mikerice marked 4 inline comments as done.

Addressed review comments.

Thanks for taking a look at this.

clang/lib/Parse/ParseOpenMP.cpp
1453

Possibly yes. If we return true we should cleanup the tokens before returning.
Returning false is probably okay here but we could end up with a bunch of cascading errors. I guess I prefer the former so I updated to that for a look.

clang/lib/Sema/SemaOpenMP.cpp
7109

I don't think so, or maybe I don't understand the question. They can specify only function parameters here and anything else is an error. It is also an error if the function parameter is specified more than once.

aaron.ballman added inline comments.Apr 9 2021, 5:01 AM
clang/lib/Sema/SemaOpenMP.cpp
7109

I don't think so, or maybe I don't understand the question.

It's most likely my near-complete ignorance of OpenMP showing through. :-D

They can specify only function parameters here and anything else is an error.

Ah, I was thinking they could refer to a member of a parameter. e.g., void foo(SomeStruct S); and they could refer to S.Whatever. But it sounds like that's explicitly not allowed?

mikerice updated this revision to Diff 336478.Apr 9 2021, 8:35 AM

Remove unused variable.

clang/lib/Sema/SemaOpenMP.cpp
7109

Right, that's not allowed.

ABataev added inline comments.Apr 9 2021, 8:43 AM
clang/include/clang/Basic/OpenMPKinds.h
173

OMPC_ADJUST_ARGS_unknown,?

clang/lib/Sema/SemaOpenMP.cpp
7073–7074

Why MutableArrayRef, not just ArrayRef?

mikerice updated this revision to Diff 336494.Apr 9 2021, 9:23 AM
mikerice marked 2 inline comments as done.

Addressed review comments, fixed switch warning.

cchen added a subscriber: cchen.Apr 15 2021, 11:17 AM

Ping. @jdoerfert any comments on this? Anyone else?

ABataev added inline comments.May 4 2021, 12:11 PM
clang/lib/Parse/ParseOpenMP.cpp
1406–1413

Maybe turn it into while {} form? What if the clause is early terminated?

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
484–491

No need for outer if

492–499

No need for outer if

mikerice updated this revision to Diff 342894.May 4 2021, 4:01 PM
mikerice marked an inline comment as done.

Addressed review comments.

clang/lib/Parse/ParseOpenMP.cpp
1406–1413

Rewrote this to parse the clauses in the loop here instead of the separate function, which seems better now that there is more than one clause possible.

aaron.ballman added inline comments.May 5 2021, 5:24 AM
clang/lib/Parse/ParseOpenMP.cpp
1447–1448

I know this is code that existed previously, but why the while loop? This strikes me as a potential infinite loop because SkipUntil returns false when the token is not found, so if the token is not found the first time, I'd be surprised if a second pass at it finds the token.

2051

Same question here as above.

3987–3990

ExpectAndConsume?

ABataev added inline comments.May 5 2021, 5:29 AM
clang/lib/Parse/ParseOpenMP.cpp
1447–1448

It may return early because of parens/braces etc. counting. In this case, it will return false while if annot_pragma_openmp_end is found, it returns true. We need to iterate to the end of the directive here, that's why there is a loop.

aaron.ballman added inline comments.May 5 2021, 5:43 AM
clang/lib/Parse/ParseOpenMP.cpp
1447–1448

Thank you for the explanation -- that sure sounds like a bug with SkipUntil to me, and looking through the code base, the only code that seems to do this dance is OpenMP code. Do you happen to have a code example that requires the loop behavior?

(Not suggesting a change needs to be made in this patch, I'm mostly wondering if there's a change we can make in a follow-up and remove these loops.)

ABataev added inline comments.May 5 2021, 5:47 AM
clang/lib/Parse/ParseOpenMP.cpp
1447–1448

OpenMP implementation guarantees that the directives are started by annot_pragma_openmp_begin and terminated by annot_pragma_openmp_end. That's why we always trying to find the terminating annot_pragma_openmp_end to recover the parsing errors in OpenMP correctly, so the parser could start parsing regular C/C++ code rather than starting parsing in the middle of the OpenMP directive.
There are some tests in test/OpenMP directory that checks for this but I don't remember which one exactly.
I think SkipUntil works correctly for the regular C/C++ code, it is just a special corner case for the pragmas

aaron.ballman added inline comments.May 5 2021, 6:34 AM
clang/lib/Parse/ParseOpenMP.cpp
1447–1448

OpenMP implementation guarantees that the directives are started by annot_pragma_openmp_begin and terminated by annot_pragma_openmp_end. That's why we always trying to find the terminating annot_pragma_openmp_end to recover the parsing errors in OpenMP correctly, so the parser could start parsing regular C/C++ code rather than starting parsing in the middle of the OpenMP directive.

My expectation is that calling SkipUntil(annot_pragma_openmp_end) should skip until it hits the given annotation token, the end of directive marker, or the end of file marker. It should not require a while loop to reach the given token properly. Either the call returns true because it found the token, or it returns false and it advanced the token to some reasonable stopping point that I would expect is outside of the stream of directive tokens.

Given that there is a guarantee that the directive ends with a annot_pragma_openmp_end, I don't see why a loop should be necessary for callers (one may be necessary within SkipUntil, obviously).

ABataev added inline comments.May 5 2021, 6:37 AM
clang/lib/Parse/ParseOpenMP.cpp
1447–1448

We need to disable braces/parens counting in SkipUntil for some cases to avoid loops.

mikerice updated this revision to Diff 379196.Oct 12 2021, 3:07 PM

Sorry for the long delay. Rebased and addressed comments.

aaron.ballman accepted this revision.Oct 13 2021, 4:58 AM

LGTM aside from some nits.

clang/lib/Parse/ParseOpenMP.cpp
1447–1448

That continues to sound like a bug with SkipUntil() to me, but one that's orthogonal to this patch.

3989

You should fix the formatting issue.

clang/lib/Sema/SemaOpenMP.cpp
7123
This revision is now accepted and ready to land.Oct 13 2021, 4:58 AM
mikerice marked 2 inline comments as done.Oct 13 2021, 9:43 AM
This revision was landed with ongoing or failed builds.Oct 13 2021, 9:43 AM
This revision was automatically updated to reflect the committed changes.
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptOct 13 2021, 9:43 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript