Page MenuHomePhabricator

[mlir][OpenMP] Add memory_order clause tests
ClosedPublic

Authored by shraiysh on May 23 2022, 4:56 AM.

Details

Summary

This patch adds tests for memory_order clause for atomic update and
capture operations. This patch also adds a check for making sure that
the operations inside and omp.atomic.capture region do not specify the
memory_order clause.

Diff Detail

Event Timeline

shraiysh created this revision.May 23 2022, 4:56 AM
shraiysh requested review of this revision.May 23 2022, 4:56 AM

Can you add a reason for this check, assuming it is specific for MLIR since the MLIR atomic capture operation can have another MLIR atomic operation inside?

Can you add a reason for this check, assuming it is specific for MLIR since the MLIR atomic capture operation can have another MLIR atomic operation inside?

According to the syntax (C/C++/Fortran), it is impossible to specify different memory order clause for the two operations within capture region or to specify it for one without specifying for the other. Hence, for the capture operation, the children are not allowed to have memory_order clause. This is similar to the already implemented hint check for the operations under omp.atomic.capture. For hint too, it is not possible to specify hints for child operations of a capture operation.

peixin accepted this revision.May 28 2022, 8:23 AM

LGTM

I have one doubt about the memory_order? It's not related to this patch. Are they implemented in codegen? I can be sure the codegen of the hint clause for atomic construct is not implemented. There was one list in clang to track the openmp status. They marked hint clause for atomic construct as done, but the link PR only has frontend support.

This revision is now accepted and ready to land.May 28 2022, 8:23 AM

Are they implemented in codegen?

I have added it here, however it has not been tested end-to-end by execution so I cannot be too sure if it works properly. The values in switch case were based on how clang handled memory order clause.

Thanks @peixin for the review. @kiranchandramohan as you commented earlier about the rule, is this patch okay with you?

This revision was automatically updated to reflect the committed changes.