Page MenuHomePhabricator

[Flang][openmp] Added semantic checks for atomic construct with update clause
Needs ReviewPublic

Authored by NimishMishra on Sep 29 2021, 6:47 AM.

Details

Summary

The following patch implements the following semantic checks for atomic construct based on OpenMP 5.0 specifications.

  • In atomic update statement, binary operator is one of +, *, -, /, .AND., .OR., .EQV., or .NEQV.

In llvm-project/flang/lib/Semantics/check-omp-structure.cpp, a new class OmpAtomicConstructChecker is added for all semantic checks implemented in this patch. This class is used in a parser::Walk in OmpStructureChecker::CheckOmpAtomicConstructStructure in check-omp-structure.cpp itself.

The entire aim is to, for any OpenMP atomic update statement, initiate a parser::Walk on the construct, capture the assignment statement in bool Pre(const parser::AssignmentStmt &assignment) inside OmpAtomicConstructChecker, and initiate a check with CheckOperatorValidity for the operator validity.

CheckOperatorValidity has two HasMember checks. The first check is to see whether the assignment statement has a binary operator or not. Because if it doesn't, the call to CompareNames is unsuccessful. The second check is to see if the operator is an allowed binary operator.

  • In atomic update statement, the statements must be of the form x = x operator expr or x = expr operator x

The CompareNames function called from within CheckOperatorValidity checks that if we have an assignment statement with a binary operator, whether the variable names are same on the LHS and RHS.

  • In atomic update statement, only the following intrinsic procedures are allowed: MAX, MIN, IAND, IOR, or IEOR

The functionality for the same is added in the function CheckProcedureDesignatorValidity.


Alternative approaches tried which we could discuss upon:

  • In CheckOperatorValidity, I earlier tried a std::visit approach. But that required us to enumerate all dis-allowed binary operators. Because had we emitted errors for anything that is not allowed, we began facing issues with assignment statements in atomic READ statements.
  • In llvm-project/flang/include/flang/Parser/parse-tree.h, the struct Expr has a variant where all relevant operators and procedure indirection things are present. One interesting structure is IntrinsicBinary which is the base structure for all relevant binary operators in parser::Expr. However, we found no way to capture the different binary operators through this base structure. Concretely, I was thinking something like this:

std::visit(common::visitors{

[&](const parser::Expr::IntrinsicBinary &x){
        // check for validity of variable names as well as operator validity   
    },
[&](const auto &x){
        // got something other than a valid binary operator. Let it pass. 
    },

}, node);

Diff Detail

Event Timeline

NimishMishra created this revision.Sep 29 2021, 6:47 AM
NimishMishra requested review of this revision.Sep 29 2021, 6:47 AM
clementval added inline comments.Sep 29 2021, 8:53 AM
flang/lib/Semantics/check-omp-structure.cpp
13

Same as in the other patch. Looks like you have an editor config that remove blank line here. Please just keep it.

42

Might be cleaner in its own file?

flang/lib/Semantics/check-omp-structure.h
232

Nit: Other checks do not mention Omp.

kiranchandramohan requested changes to this revision.Sep 29 2021, 3:07 PM

Thanks for this patch. I have some suggestions, please have a look and let me know what you think.

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

I think this class and associated walk is probably not required. Additional walks and its instantiation adds to compilation time and should be used only as a last resort.

In the Atomic update/Atomic case, the AssignmentStatement can be obtained directly from the tuple since it is a part of the tuple inside the struct OmpAtomicUpdate or struct OmpAtomic nodes. Once you can extract the assignment, you can perform the checks that are done here inside the OmpStructureChecker class itself.

// ATOMIC UPDATE
struct OmpAtomicUpdate {
  ...
  std::tuple<OmpAtomicClauseList, Verbatim, OmpAtomicClauseList,
      Statement<AssignmentStmt>, std::optional<OmpEndAtomic>> t;
};
55

We have some similar code in reduction checking. Can you check the following links and see whether it will be beneficial to write CheckProcedureDesignatorValidity and CheckOperatorValidity in a similar way?

https://github.com/llvm/llvm-project/blob/28981015526f2192440c18f18e8a20cd11b0779c/flang/lib/Semantics/check-omp-structure.cpp#L1415
https://github.com/llvm/llvm-project/blob/28981015526f2192440c18f18e8a20cd11b0779c/flang/lib/Semantics/check-omp-structure.cpp#L1446

This revision now requires changes to proceed.Sep 29 2021, 3:07 PM

Thank you for the comments. Please find my views on the same and let me know what you think about this

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

It is indeed possible to shift this class to a new file or entirely replace this class by functions instead.

However, the reason to go ahead with this approach was to imitate similar style in check-omp-structure.cpp. Two of the classes: OmpWorkshareBlockChecker and OmpCycleChecker are instantiated only once in the source but still exist as classes rather than a set of independent functions. This was the motivation behind defining a class for my checks as well.

55

Writing like https://github.com/llvm/llvm-project/blob/28981015526f2192440c18f18e8a20cd11b0779c/flang/lib/Semantics/check-omp-structure.cpp#L1446 won't be possible as far as I can see. Reason being instead of working with parser::DefinedOperator, we are rather working with operators defined in parser::Expr like parser::Expr::Add etc.

Writing like https://github.com/llvm/llvm-project/blob/28981015526f2192440c18f18e8a20cd11b0779c/flang/lib/Semantics/check-omp-structure.cpp#L1415 should be possible. I will rewrite the check accordingly

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

I think a workshare construct can contain arbitrary blocks of code and hence it is difficult to extract out an Assignment statement or expression from there. But in the atomic construct case, the region it contains is much simpler and it is easy to extract out the Assignment Statement from the atomic construct itself.

So I would recommend implementing as functions unless you have a strong objection.

55

OK. Thanks.

NimishMishra added inline comments.Oct 8 2021, 1:31 AM
flang/lib/Semantics/check-omp-structure.cpp
42

No I think you raised an important point. I will remove the class and write separate functions instead.

NimishMishra marked 7 inline comments as done.
kiranchandramohan requested changes to this revision.Oct 11 2021, 2:42 PM

Can the title of the patch be changed to "Semantic checks for atomic construct with update clause"?

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

I think lvalue, rvalue are not commonly used terms in Fortran. Also, is this applicable for update atomic only? Is that enforced?

A possible error message could be something like the following,
Atomic update variable <var-name> not found in the RHS of the assignment statement in an ATOMIC UPDATE construct.

1323

Is the template required here? Can you pass the AssignmentStatement instead here?

This revision now requires changes to proceed.Oct 11 2021, 2:42 PM
NimishMishra added inline comments.Oct 11 2021, 10:28 PM
flang/lib/Semantics/check-omp-structure.cpp
1311

Yes, this particular variable name mismatch in RHS and LHS is applicable to atomic update statements only.

I will change the error message.

1323

It's possible. However, it would require extracting Verbatim and AssignmentStmt in void OmpStructureChecker::Enter(const parser::OpenMPAtomicConstruct &x) at two places: parser::OmpAtomicUpdate and parser::OmpAtomic

I put a template in this so that for both OmpAtomicUpdate and OmpAtomic, we have refactored code where the directive and assignment statement extractions are present in source code at one place.

If there is no problem with duplication of these two extractions, I will remove this template and receive assignment statements instead.

peixin added a comment.Sun, Nov 7, 7:52 PM

I think the following check is not correct. I guess you referred to C/C++ code format.

In atomic update statement, the statements must be of the form x = x operator expr or x = expr operator x.

According to OpenMP 5.0 Spec 2.17.7 at page 238, the update-statement has one of the following forms:
x = x operator expr
x = expr operator x
x = intrinsic_procedure_name (x, expr_list)
x = intrinsic_procedure_name (expr_list, x)

@peixin Changes done.
@kiranchandramohan I have updated the error statements per your comments.

NimishMishra retitled this revision from [Flang][openmp] Added semantic checks for atomic construct to [Flang][openmp] Added semantic checks for atomic construct with update clause.Thu, Nov 25, 9:15 PM