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

[NimishMishra]

Summary

This patch adds semantic checks for atomic capture construct.

Depends on D127620

[NimishMishra]

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

[shraiysh]

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]

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.

[kiranchandramohan]

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

1498

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

[peixin]

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"

2. 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.

[NimishMishra]

In D126612#3550182, @peixin wrote:

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]

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]

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.