This is an archive of the discontinued LLVM Phabricator instance.

[Clang][OpenMP] Add parse and sema for iterator map modifier
ClosedPublic

Authored by doru1004 on Jan 16 2023, 1:03 PM.

Details

Summary

This patch adds parse and sema support for iterator map modifier.

Diff Detail

Event Timeline

doru1004 created this revision.Jan 16 2023, 1:03 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 16 2023, 1:03 PM
doru1004 requested review of this revision.Jan 16 2023, 1:03 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 16 2023, 1:03 PM
ABataev added inline comments.Jan 16 2023, 1:34 PM
clang/include/clang/AST/OpenMPClause.h
5780

We're using tail allocation for such objects.

doru1004 updated this revision to Diff 489787.Jan 17 2023, 5:54 AM
doru1004 marked an inline comment as done.

Erroneous test are required

clang/include/clang/AST/OpenMPClause.h
310–314

Why do we need it here, if it is specific to map clause?

317

Why ArrayRef if only one expression is expected?

5914–5916

Just getIteratorRef() should be enough.

clang/lib/Sema/SemaOpenMP.cpp
1166–1169

Use llvm::any_of()

doru1004 updated this revision to Diff 489917.Jan 17 2023, 12:29 PM
doru1004 marked 3 inline comments as done.
doru1004 marked an inline comment as done.Jan 17 2023, 12:31 PM
ABataev added inline comments.Jan 17 2023, 12:45 PM
clang/include/clang/AST/OpenMPClause.h
5866–5867

Can we use just a nullptr as a mark that there is no associated iterator?

clang/include/clang/Parse/Parser.h
3474–3475 ↗(On Diff #489917)

Where is it defined, cannot find it in the patch?

clang/lib/Sema/SemaOpenMP.cpp
1166–1168

Do you really need to store the variable in the stack, is not it enough just to check that the type of this variable is BuiltinType::OMPIterator?

clang/test/OpenMP/target_map_messages.cpp
256–257

Cases with incorrect parsing, wrong variables etc.?

doru1004 updated this revision to Diff 489942.Jan 17 2023, 2:21 PM
doru1004 marked 3 inline comments as done.
doru1004 added inline comments.
clang/include/clang/Parse/Parser.h
3474–3475 ↗(On Diff #489917)

Leftover, removed it.

clang/lib/Sema/SemaOpenMP.cpp
1166–1168

I'm happy to replace this if you think it will work. Is there an example somewhere in the code where I can get from the VarDecl to the build in type you mention?

ABataev added inline comments.Jan 17 2023, 2:23 PM
clang/lib/Sema/SemaOpenMP.cpp
1166–1168

You have already a check IteratorModifier->getType()->isSpecificBuiltinType(BuiltinType::OMPIterator), you can you something similar for the variable

doru1004 updated this revision to Diff 490188.Jan 18 2023, 8:39 AM
doru1004 marked 2 inline comments as done.
ABataev added inline comments.Jan 18 2023, 8:44 AM
clang/include/clang/AST/OpenMPClause.h
5779–5780

It can be removed

clang/test/OpenMP/target_map_messages.cpp
970–979

Test cases for wrong variables in mappers?

doru1004 added inline comments.Jan 18 2023, 8:52 AM
clang/test/OpenMP/target_map_messages.cpp
970–979

You mean as part of the iterator ? like iterator(it = 0:UndefVar) ?

ABataev added inline comments.Jan 18 2023, 9:02 AM
clang/test/OpenMP/target_map_messages.cpp
970–979

I mean, you have a check that in mappers only iterator vars are allowed. Can you add a check for this?

doru1004 added inline comments.Jan 18 2023, 9:12 AM
clang/test/OpenMP/target_map_messages.cpp
970–979

As far as I understand it the additional check I added to the existing mapper checks is needed because of the way the mapper check was written. The mapper checks looks at declarations and if a mapper clause exists then it assumes that the declaration must be coming from that mapper clause. This used to hold in the past since that was the only way you could have a declaration. This is not true anymore since we can now have declarations coming from both the mapper and the iterator modifier. I'll add a test to showcase this.

doru1004 updated this revision to Diff 490277.Jan 18 2023, 1:30 PM
doru1004 marked 2 inline comments as done.
doru1004 added inline comments.
clang/lib/Sema/SemaOpenMP.cpp
1166–1168

This didn't work and I had to revert to using the stack!

ABataev added inline comments.Jan 18 2023, 1:36 PM
clang/lib/Sema/SemaOpenMP.cpp
1166–1168

Why?

doru1004 added inline comments.Jan 18 2023, 2:51 PM
clang/lib/Sema/SemaOpenMP.cpp
1166–1168

I checked the output of the check and it was false when it should have been true! If you check the latest test that I added it showcases the source code and in the case of OpenMP 5.2 you can see that the message "only variable 'vvec' is allowed in map clauses of this 'omp declare mapper' directive" doesn't appear when a legal iteration variable is used.
If I used the check you suggested then the error message appears.
In the example you pasted the check is performed on a Expr *. In the case here, we only have VD which is a VarDecl.

ABataev added inline comments.Jan 18 2023, 3:05 PM
clang/lib/Sema/SemaOpenMP.cpp
1166–1168

Did not get it. It still shall be of type builtintype::OMPIterator.

doru1004 added inline comments.Jan 18 2023, 3:13 PM
clang/lib/Sema/SemaOpenMP.cpp
1166–1168

I am not sure how I can force it to have that type when it just doesn't. Do you have any suggestions?

doru1004 added inline comments.Jan 18 2023, 3:18 PM
clang/lib/Sema/SemaOpenMP.cpp
1166–1168

The VD that we are checking for this builtin is coming from somewhere else in the code, it is passed into the Sema::DiagnoseUseOfDecl( function. It's not a VarDecl that is under the control of anything added in this patch.

doru1004 added inline comments.Jan 18 2023, 3:25 PM
clang/lib/Sema/SemaOpenMP.cpp
1166–1168

This implementation is in line with the current checks for the declaration of the mapper variable. You store the declaration onto the stack so that you can compare it with the incoming VarDecl passed to the diagnose function.

doru1004 added inline comments.Jan 18 2023, 4:17 PM
clang/lib/Sema/SemaOpenMP.cpp
1166–1168

Some debug printouts regarding VD:

VD->dump();

VarDecl 0x55b57f81dec8 <test_declare_mapper_iterator.c:26:52> col:52 implicit used it 'int'

This is the type of the variable if you do VD->getType()->dump():

BuiltinType 0x55b57f6d2560 'int'
doru1004 added inline comments.Jan 18 2023, 4:23 PM
clang/lib/Sema/SemaOpenMP.cpp
22400–22413

This input to this function is the VD variable I've been talking about. If you print it all out it's just a simple VarDecl:

if you do VD->dump();

VarDecl 0x55b57f81dec8 <test_declare_mapper_iterator.c:26:52> col:52 implicit used it 'int'

if you do VD->getType()->dump():

BuiltinType 0x55b57f6d2560 'int'

Can you add positive test for declare mapper?

clang/lib/Sema/SemaExpr.cpp
5423–5426

Can we register this variable only in declare mapper context, i.e. add a check that we add it only for declare mapper?

doru1004 added inline comments.Jan 19 2023, 9:45 AM
clang/test/OpenMP/declare_mapper_messages.c
33–36

Here we have both the positive and the negative declare mapper cases. Please let me know if you meant something different.

doru1004 added inline comments.Jan 19 2023, 9:47 AM
clang/lib/Sema/SemaExpr.cpp
5423–5426

Happy to do that!

ABataev added inline comments.Jan 19 2023, 9:49 AM
clang/test/OpenMP/declare_mapper_messages.c
33–36

Would be good to have ast print case and codegen

doru1004 added inline comments.Jan 19 2023, 9:53 AM
clang/test/OpenMP/declare_mapper_messages.c
33–36

I can add ast print! Code gen for this map modifier is going to be done in a separate patch.

ABataev added inline comments.Jan 19 2023, 9:54 AM
clang/test/OpenMP/declare_mapper_messages.c
33–36

Ok, a st print should be enough

doru1004 updated this revision to Diff 490659.Jan 19 2023, 2:18 PM
doru1004 marked 5 inline comments as done.
This revision is now accepted and ready to land.Jan 20 2023, 4:23 AM