This is an archive of the discontinued LLVM Phabricator instance.

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

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 or x = intrinsic_procedure_name (expr_list, x) or x = intrinsic_procedure_name (x, expr_list)

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.

  • At most one memory-order-clause may appear on the construct.

The overloaded function void OmpStructureChecker::CheckAtomicMemoryOrderClause checks for this through a simple counting of memory order clauses.


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
230

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
1226

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.

1238

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
1226

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

I will change the error message.

1238

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.Nov 7 2021, 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.Nov 25 2021, 9:15 PM

Thanks for the patch Nimish. I had a few minor comments about testcases. Rest LGTM.

flang/test/Semantics/omp-atomic03.f90
97

MAX and MIN can have more than 2 arguments (not sure about the rest). Maybe add test cases for them.

flang/test/Semantics/omp-atomic04.f90
100

Should probably be !$omp atomic update here.

kiranchandramohan edited reviewers, added: klausler; removed: ftynse, Meinersbur.Dec 9 2021, 5:40 AM
kiranchandramohan added a subscriber: klausler.

Adding Peter for expert opinion. Removing some reviewers (Alex, Michael) who are probably not interested in this patch.

flang/lib/Semantics/check-omp-structure.cpp
1230–1234

Would return common::HasMember<T, AllowedBinaryOperators>; work here?

1238

The name of the function suggests it is checking something, so I think it is better for it to not update state. So let us move the following code back to the parent function, remove the template and only pass the assignment statement.

const auto &dir{std::get<parser::Verbatim>(construct.t)};
PushContextAndClauseSets(dir.source, llvm::omp::Directive::OMPD_atomic);
1252–1253

This can be sunk into the else if part.

1272

I think this will not probably work. Consider the example below. Maybe we should compare the symbols here.

Also in general, it might be better to work with the Evaluate Expression framework here.

Adding @klausler for an expert opinion.

program OmpAtomic
   use omp_lib
   type simple
     integer :: z
   end type
   real x
   integer :: y, z
   type(simple) ::s

   z = 1

  !$omp atomic
   z = IAND(s%z, 4)
end
flang/test/Semantics/omp-atomic03.f90
42–88

I think you have added a lot of invalid intrinsic procedure name test. While testing more is mostly the right approach, I think in this case we don't get much out of it. Can you reduce this to one or two?

peixin added a comment.Dec 9 2021, 7:44 AM

The following description is not correct.

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

Please check my previous comments:
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)

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

CheckOperatorValidity -> IsOperatorValid considering that the "true" is returned when the operator is valid.

1226

ATOMIC UPDATE -> ATOMIC (UPDATE)?

1237

Please add one blank line after line 1322.

1261

ATOMIC construct UPDATE -> ATOMIC (UPDATE)?

1289

ATOMIC construct UPDATE -> ATOMIC (UPDATE)?

flang/test/Semantics/omp-atomic02.f90
23

Can you add a blank line between every two test cases? It's hard to read these test cases.

NimishMishra marked 14 inline comments as done.
NimishMishra edited the summary of this revision. (Show Details)
NimishMishra edited the summary of this revision. (Show Details)

@NimishMishra Did you address all my previous comments? If yes, I will take a look at this patch again.

@NimishMishra Did you address all my previous comments? If yes, I will take a look at this patch again.

Yes. There were suggestions regarding changing the format of update statement in description as well as certain changes in the error reporting. I have fixed them.

peixin accepted this revision.Jan 6 2022, 2:45 AM

LGTM

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

Nit: It's strange that clang-tidy does not report the error for this.

"Atomic update variable '%s' not found in the RHS of the assignment "
"statement in an ATOMIC (UPDATE) construct"_err_en_US,
1247

Nit: Should we use the following format?

!(name->source == "max" || name->source == "min" ||
  name->source == "iand" || name->source == "ior" ||
  name->source == "ieor")) {
1249

Nit: https://llvm.org/docs/CodingStandards.html#source-code-width

"Invalid intrinsic procedure name in OpenMP ATOMIC (UPDATE) "
"statement"_err_en_US);
1275

Nit: -> 80 columns?

1313

Using lambda for the error message may be better.

peixin added a comment.Mar 1 2022, 3:03 AM

I noticed you landed this patch without addressing those code style issues I commented above such as https://github.com/llvm/llvm-project/blob/aeab6167b0a13ab3a46798c0a9929458e0413f08/flang/lib/Semantics/check-omp-structure.cpp#L632. Also, when you land the patch, you should add the commit message and the review link, which will close this PR for you automatically.

Can you create one new NFC patch to fix them and close this PR?

I noticed you landed this patch without addressing those code style issues I commented above such as https://github.com/llvm/llvm-project/blob/aeab6167b0a13ab3a46798c0a9929458e0413f08/flang/lib/Semantics/check-omp-structure.cpp#L632. Also, when you land the patch, you should add the commit message and the review link, which will close this PR for you automatically.

Can you create one new NFC patch to fix them and close this PR?

Hi @peixin. Thanks for bringing this to attention. I actually addressed the comments through formatting. I must have made a mistake while landing it (which also didn't close this differential). I'll fix it.

Herald added a project: Restricted Project. · View Herald TranscriptMar 7 2022, 8:38 PM
NimishMishra accepted this revision.Mar 8 2022, 4:30 AM

Closing this as the formatting related changes are now covered in the NFC: https://reviews.llvm.org/D121186

NimishMishra abandoned this revision.Mar 8 2022, 5:27 AM

Abandoning this as the commit https://reviews.llvm.org/rG3519dcfec22963fbb84e154cecc2df22e6c7724f has merged it. Further formatting changes are covered in NFC https://reviews.llvm.org/D121186