This is an archive of the discontinued LLVM Phabricator instance.

[flang][openacc] Make sure the correct flags are set for symbol in acc declare
ClosedPublic

Authored by clementval on Aug 3 2023, 2:56 PM.

Details

Summary

Flags were not correctly set for symbols appearing in the OpenACC declare
directive in module declaration part.

Also some missing flags for OpenACC are added. This makes the Flags enum

64 and then the implementation switch to std::bitset as defined in

flang/include/flang/Common/enum-set.h. Therefore, constexpr cannot be
used for set of flags in flang/lib/Semantics/resolve-directives.cpp.

Diff Detail

Event Timeline

clementval created this revision.Aug 3 2023, 2:56 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
Herald added a subscriber: Anastasia. · View Herald Transcript
clementval requested review of this revision.Aug 3 2023, 2:56 PM
razvanlupusoru added inline comments.Aug 4 2023, 11:31 AM
flang/lib/Semantics/resolve-directives.cpp
237

Should present be a data sharing attribute instead of data mapping attribute?

flang/test/Semantics/OpenACC/acc-symbols01.f90
10 ↗(On Diff #547012)

What should happen if you have another clause for the same variable?
aka another acc parallel region with copyin(c).

Seems a bit wrong to have this flag on the symbol unless it is in a declare clause.

clementval added inline comments.Aug 4 2023, 11:38 AM
flang/test/Semantics/OpenACC/acc-symbols01.f90
10 ↗(On Diff #547012)

You would have two attributes on the symbol.

clementval added inline comments.Aug 4 2023, 11:40 AM
flang/lib/Semantics/resolve-directives.cpp
237

I can move it. This set is only used to check multiple occurrences.

clementval added inline comments.Aug 4 2023, 11:52 AM
flang/test/Semantics/OpenACC/acc-symbols01.f90
10 ↗(On Diff #547012)

I'll update the code to add attribute for declare only (at least for now) as it makes more sense.

razvanlupusoru added inline comments.Aug 4 2023, 11:53 AM
flang/test/Semantics/OpenACC/acc-symbols01.f90
10 ↗(On Diff #547012)

I agree.

Address review comments

razvanlupusoru accepted this revision.Aug 4 2023, 12:05 PM

Thank you!

This revision is now accepted and ready to land.Aug 4 2023, 12:05 PM
clementval updated this revision to Diff 547320.Aug 4 2023, 1:07 PM

clang-format

This revision was landed with ongoing or failed builds.Aug 4 2023, 1:08 PM
This revision was automatically updated to reflect the committed changes.