This is an archive of the discontinued LLVM Phabricator instance.

[OPENMP51] Add present modifier in defaultmap clause
ClosedPublic

Authored by cchen on Dec 1 2020, 1:46 PM.

Details

Summary

Support present modifier in defaultmap by adding an extra dimension
for ImplicitMap. Therefore, we now create OMPMapClause in ActOnOpenMPExecutableDirective
based on both maptype and maptype-modifier.

Diff Detail

Event Timeline

cchen created this revision.Dec 1 2020, 1:46 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 1 2020, 1:46 PM
cchen requested review of this revision.Dec 1 2020, 1:46 PM
cchen updated this revision to Diff 308760.Dec 1 2020, 1:52 PM

Rebase and fix coding style

cchen added a comment.Dec 1 2020, 1:56 PM

I now only update target_defaultmap_messages test since I'm not sure what I've done is correct for the new version. Will add all other defaultmap_messages tests once corrected the stuff I added in target_defaultmap_messages.

ABataev added inline comments.Dec 2 2020, 1:51 PM
clang/lib/Sema/SemaOpenMP.cpp
3310

Add a reference to the OpenMP standard, section and related text.

3491

Why need to add an empty SourceLocation?

18592–18593

You can make it StringRef

18594

Same, StringRef

18609–18623

StringRef

clang/test/OpenMP/target_defaultmap_codegen.cpp
1557

Add in a separate test file

clang/test/OpenMP/target_defaultmap_messages.cpp
5

Why this line is changed?

cchen added inline comments.Dec 2 2020, 2:13 PM
clang/lib/Sema/SemaOpenMP.cpp
3491

I'm doing this since there is no actual location for present modifier. Maybe I should just pass llvm::None to ActOnOpenMPMapClause.

cchen updated this revision to Diff 309292.Dec 3 2020, 9:55 AM

Update based on comment

ABataev added inline comments.Dec 3 2020, 10:23 AM
clang/lib/Sema/SemaOpenMP.cpp
3491

Could you store the original location of the present modifier and add a real source location rather. Better to use correct locations when possible, it may help with the debug info emission

cchen updated this revision to Diff 309352.Dec 3 2020, 1:28 PM

Add more tests and store location for present modifier

ABataev added inline comments.Dec 10 2020, 8:43 AM
clang/test/OpenMP/target_defaultmap_codegen.cpp
1523

Why the order has changed?

1524

Looks like this comment is outdated and must be fixed.

cchen added inline comments.Dec 11 2020, 1:10 PM
clang/test/OpenMP/target_defaultmap_codegen.cpp
1523

This is because, in SemaOpenMP line 5176, I'm invoking ActOnOpenMPMapClause in a fixed order. The outer dimension of the 2-dim for loop is the variable category of defaultmap (scalar -> aggregate -> pointer), therefore, 4096 (aggregate, Vector) is not happening after 4 (scalar, a).

1524

Going to fix this, thanks.

cchen updated this revision to Diff 311305.Dec 11 2020, 1:13 PM

Fix outdated comment in test

This revision is now accepted and ready to land.Dec 15 2020, 10:09 AM
This revision was landed with ongoing or failed builds.Dec 15 2020, 11:50 AM
This revision was automatically updated to reflect the committed changes.