This is an archive of the discontinued LLVM Phabricator instance.

[flang][OpenMP] Added semantic checks for atomic capture construct
Changes PlannedPublic

Authored by NimishMishra on May 29 2022, 4:46 AM.

Details

Summary

This patch adds semantic checks for atomic capture construct.

Depends on D127620

Diff Detail

Event Timeline

NimishMishra created this revision.May 29 2022, 4:46 AM
Herald added a project: Restricted Project. · View Herald Transcript
NimishMishra requested review of this revision.May 29 2022, 4:46 AM

There are still some semantic checks left. I will cover them in a separate patch

NimishMishra edited the summary of this revision. (Show Details)May 29 2022, 6:02 AM
NimishMishra edited the summary of this revision. (Show Details)May 29 2022, 6:05 AM

Two minor comments at the moment. Will review it later this week.

flang/lib/Semantics/check-omp-structure.cpp
1498

Can we please start adding some documentation to the function right above it, like here

1806–1821

nit: wrap this loop in a lambda inside this function and use twice instead of duplication.

To avoid higher cyclomatic complexity, use early return in the lambda function when the if conditions become nullptr. What happens if none of the clauses in the list match the if conditions? Is it possible to have one clause match some conditions and not the rest? If not, then we should assert instead of if and if yes, should their else parts be ignored? (I think the innermost if must always be true if control reaches there, but please feel free to correct me).

NimishMishra added inline comments.May 29 2022, 7:55 AM
flang/lib/Semantics/check-omp-structure.cpp
1498

This would certainly be nice. If others agree too, we can go ahead and document the functions.

flang/lib/Semantics/check-omp-structure.cpp
1498

+1 for more documentation, particularly for special support functions.

peixin added a comment.EditedJun 1 2022, 7:30 AM

Can you split this patch? The small patch with single function is easier to review. If there are multiple checks with similar style, you can put them together.

  1. check "capture statement is of the form v = x if atomic construct is read or capture"

check "write statement is of the form x = expr if atomic construct is write"

  1. check "x must not have the ALLOCATABLE attribute."

check "operator must refer to the intrinsic operator and not to a user-defined operator"
check "hint-expression is a constant expression that evaluates to a scalar value with kind omp_sync_hint_kind and a value that is a valid synchronization hint."

Or even split point 2 into three small patches.

flang/lib/Semantics/check-omp-structure.cpp
1498

+1

But I prefer not to add the comments like this The comments for op, converter, loc, eval are useless. My preference is to use function name and argument name comment themselves. If they cannot comment itself such as with some special support, it's better to add comments for them.

Can you split this patch? The small patch with single function is easier to review. If there are multiple checks with similar style, you can put them together.

Sure. Working on splitting this patch's contents across multiple smaller patches

NimishMishra retitled this revision from [flang][OpenMP] Add OpenMP 5.0 based semantic checks for atomic construct to [flang][OpenMP] Added semantic checks for atomic capture construct.
NimishMishra edited the summary of this revision. (Show Details)
NimishMishra added inline comments.Jul 25 2022, 5:32 AM
flang/include/flang/Parser/parse-tree.h
3715

I have an idea here that could potentially ease lowering of atomic capture construct. To lower such constructs, we need to know what the two statements are before we can create operations for them.

My idea was if we could introduce attributes as these in the parse-tree node itself which shall be filled in during semantics. Then during lowering, we just look at Stmt1Type and Stmt2Type and create required operations for the two statements of capture construct.

The only caveat is that the node is const at the point where OmpStructureChecker is instantiated. Any ideas on how to go about this problem will greatly help.

NimishMishra planned changes to this revision.EditedAug 18 2022, 6:38 PM

The design of this solution could be improved. Following whatever reviews come in https://reviews.llvm.org/D127272, we can use the same basic structural checks from that patch here to understand what the statements 1 and 2 of the atomic capture construct actually are (among read/write/update statements). That would remove dependence on global boolean flags, which don't look good design wise.