Page MenuHomePhabricator

[flang][OpenMP][NFC] Refactor code related to OpenMP atomic memory order clause semantics
ClosedPublic

Authored by NimishMishra on Jun 14 2022, 8:41 PM.

Details

Summary

Refactor functions handling memory order clauses. Related tests are already available in omp-atomic01.f90, so I do not add more tests.

Diff Detail

Event Timeline

NimishMishra created this revision.Jun 14 2022, 8:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 14 2022, 8:41 PM
NimishMishra requested review of this revision.Jun 14 2022, 8:41 PM
peixin added inline comments.Jun 15 2022, 6:40 AM
flang/lib/Semantics/check-omp-structure.cpp
1594

How about not changing CheckAtomicMemoryOrderClause and call it twice here? And also remove OmpStructureChecker::CheckAtomicMemoryOrderClause(const parser::OmpAtomicClauseList &leftHandClauseList, const parser::OmpAtomicClauseList &rightHandClauseList). Then you will escape from passing the pointer by functions and using the obscure variable names leftHandClauseList an rightHandClauseList.

CheckAtomicMemoryOrderClause(std::get<0>(atomicUpdate.t));
CheckAtomicMemoryOrderClause(std::get<2>(atomicUpdate.t));
NimishMishra added inline comments.Jun 15 2022, 6:51 AM
flang/lib/Semantics/check-omp-structure.cpp
1594

That could be done. However, then some additional handling would be required for numMemoryOrderClause.

For example, !$omp atomic <memory_order_clause> read <memory_order_clause> is invalid since there are > 1 memory order clauses here. Thus I need the information- that we found 1 memory order clause while processing the left hand clause list- while I am working on the right hand clause list.

However, I can not make numMemoryOrderClause a static variable, because then

!$omp atomic  read <memory_order_clause>
   x = y
!$omp atomic  write <memory_order_clause>
   y = 1

will start giving errors, as the variable will persist across several different atomic constructs.

I do not have a strong opinion here however. I will do it as you suggest.

peixin added inline comments.Jun 15 2022, 7:34 AM
flang/lib/Semantics/check-omp-structure.cpp
1594

!$omp atomic <memory_order_clause> read <memory_order_clause>

Sorry, I didn't notice this case. Your current approach is fine to me. I noticed the memory order check are all for atomic constructs. Maybe put the lambda in the front of void OmpStructureChecker::Enter(const parser::OpenMPAtomicConstruct &x)? This is not one hard comment.

peixin accepted this revision.Jun 15 2022, 5:57 PM

You can wait for one day before landing if no one has further comments so that we can proceed the review for other patches depending on this.

This revision is now accepted and ready to land.Jun 15 2022, 5:57 PM