Page MenuHomePhabricator

[flang][OpenMP] Added semantic checks for hint clause
ClosedPublic

Authored by NimishMishra on Jun 13 2022, 12:44 AM.

Details

Summary

This patch improves semantic checks for hint clause.

  • 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."

Depends on D127822

Diff Detail

Event Timeline

NimishMishra created this revision.Jun 13 2022, 12:44 AM
Herald added a project: Restricted Project. · View Herald Transcript
NimishMishra requested review of this revision.Jun 13 2022, 12:44 AM
peixin requested changes to this revision.Jun 14 2022, 2:31 AM

Can you split this patch into two:

  1. refactor the atomic code preparing for other semantic check patches
  2. check hint clause for both atomic and critical construct: 2.1 At most one hint clause can be specified. 2.2 The hint clause must have the non-negative constant integer expression. (Maybe I am wrong for this?)
flang/lib/Semantics/check-omp-structure.cpp
1626

I don't think the value must be 0, 1, 2, 4, 8. Please check OpenMP 5.0 2.17.12:

The hints can be combined by using the + or | operators in C/C++ or the + operator in Fortran.

You may also check the behavior of clang. The hint clause must have the non-negative constant integer expression.

This revision now requires changes to proceed.Jun 14 2022, 2:31 AM
flang/lib/Semantics/check-omp-structure.cpp
1626

+1. Also use descriptive name/ENUMs instead of hardcoded constants.

flang/lib/Semantics/check-omp-structure.cpp
1605–1610

Add tests for these.

1626

There are also some restrictions on what hints can be combined. Hints are present in the Critical construct it. It will be good if you can add a common check for hints when you split the patch.

The hints omp_sync_hint_uncontended and omp_sync_hint_contended cannot be combined.
The hints omp_sync_hint_nonspeculative and omp_sync_hint_speculative cannot be combined

https://www.openmp.org/spec-html/5.0/openmpsu100.html

NimishMishra edited the summary of this revision. (Show Details)Jun 15 2022, 12:14 AM
NimishMishra retitled this revision from [flang][OpenMP] Added semantic checks for atomic construct clauses to [flang][OpenMP] Added semantic checks for hint clause.
NimishMishra edited the summary of this revision. (Show Details)

Revised the diff to reflect hint clause changes only

Can you also support this check for critical construct?

Added support for critical construct also

Is hint_contended + hint_speculative + hint_speculative rejected in other compilers?

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

Add assert for ompClause and remove the nullptr. And no need to check hintClause.

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

Nit: Remove the empty line.

288

Is std::set better?

293

Nit: Place directiveNest_ things together. I mean, move line 293 to line 279.

flang/test/Semantics/OpenMP/omp-atomic-hint-clause.f90
9

Please add two more scenarios:

  1. integer, parameter :: a = 1
  2. abs(1)
NimishMishra added inline comments.Jun 27 2022, 6:43 AM
flang/lib/Semantics/check-omp-structure.cpp
332

This is problematic whenever OmpAtomicClauseList is used. This is because every OmpAtomicClause is a variant of OmpMemoryOrderClause and OmpClause. Means whenever memory order clauses are used:

!$omp atomic read release
   v = x

The compiler starts giving assertion failures.

peixin added inline comments.Jun 27 2022, 6:05 PM
flang/lib/Semantics/check-omp-structure.cpp
332

OK. Now I understand how your code is organized. How about the following:

if (const Fortran::parser::OmpClause::Hint *hintClause = std::get_if<Fortran::parser::OmpClause::Hint>(&ompClause->u)) {

std::optional<std::int64_t> hintValue = GetIntValue(hintClause->v);
if (hintValue) {
  checking ...
} else {
   error
}

}

335

Nit

  • Addressed comments.
  • Improved the logic to check for presence of certain bits in tandem, instead of comparing the entire hintValue to predefined integer values. Concretely, in the last nibble in the integral hintValue, 0xC and 0x3 are not allowed.

@peixin @kiranchandramohan does this look in good shape now?

peixin accepted this revision.Jul 11 2022, 7:04 PM

LGTM except for one nit.

flang/lib/Semantics/check-omp-structure.cpp
339
This revision is now accepted and ready to land.Jul 11 2022, 7:04 PM