This is an archive of the discontinued LLVM Phabricator instance.

[Flang][OpenMP][Sema] Add directive rewrite pass to support atomic_default_mem_order REQUIRES clause

Authored by skatrak on Aug 16 2023, 9:07 AM.



This patch creates the OmpRewriteMutator pass that runs at the end of RewriteParseTree(). This pass is intended to make OpenMP-specific mutations to the PFT after name resolution.

In the case of the atomic_default_mem_order clause of the REQUIRES directive, name resolution results in populating global symbols with information about the REQUIRES clauses that apply to that scope. The new rewrite pass is then able to use this information in order to explicitly set the memory order of ATOMIC constructs for which that is not already specified.

Given that this rewrite happens before semantics checks, the check of the order in which ATOMIC constructs without explicit memory order and REQUIRES directives with atomic_default_mem_order appear is moved earlier into the rewrite pass. Otherwise, these problems would not be caught by semantics checks, since the PFT would be modified by that stage.

This is patch 4/5 of a series splitting D149337 to simplify review.

Depends on D157983.

Diff Detail

Event Timeline

skatrak created this revision.Aug 16 2023, 9:07 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
skatrak requested review of this revision.Aug 16 2023, 9:07 AM
skatrak updated this revision to Diff 557238.Sep 22 2023, 5:33 AM

Rebase, fix handling of acq_rel clause to match standard and complete unit tests. Ping for review!

Looks OK to me. Three questions/comments inline.


Is this class of use currently?


We could alternatively handle everything here by looking for the variant (AtomicDefaultMemOrder) in clause.u. This way we do not have to save the clause source.


Why can't we directly invoke the following way?

parser::Walk(program, OmpRewriteMutator);
skatrak updated this revision to Diff 557757.Oct 18 2023, 6:45 AM
skatrak marked 2 inline comments as done.

Address review comments.

Thank you Kiran for the review! I hope these changes address most of your concerns.


It could be removed without much issue. The idea with it was to simplify creating an AccRewriteMutator later on, if needed, just as it's done for the {Omp,Acc}AttributeVisitor in resolve-directives.cpp. Let me know if it's best to only add it when actually needed and I'll remove it.


Thank you for the suggestion, it does look cleaner that way.





OK. you can retain for now.


Nit: Is this Walk function used now?

This revision is now accepted and ready to land.Oct 18 2023, 7:02 AM
skatrak updated this revision to Diff 557761.Oct 18 2023, 8:37 AM
skatrak marked 3 inline comments as done.

Address nit comments and replace leftover initialization parentheses for braces. Will merge this patch tomorrow, unless there are any further comments.


It's not. Good catch, thank you!