Page MenuHomePhabricator
Differential D89583 Diff 311496 flang/lib/Semantics/check-omp-structure.cpp

Changeset View

Standalone View

View Options

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

//===-- lib/Semantics/check-omp-structure.cpp -----------------------------===// //===-- lib/Semantics/check-omp-structure.cpp -----------------------------===//
// //
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information. // See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
// //
//===----------------------------------------------------------------------===// //===----------------------------------------------------------------------===//
#include "check-omp-structure.h" #include "check-omp-structure.h"
#include "flang/Parser/parse-tree.h" #include "flang/Parser/parse-tree.h"
clementvalUnsubmitted
Done
ReplyInline Actions

Is this necessary?

clementval: Is this necessary?
sameeranjoshiAuthorUnsubmitted
Done
ReplyInline Actions

For common::visitors I will see of they can be removed.

sameeranjoshi: For `common::visitors` I will see of they can be removed.
#include "flang/Semantics/tools.h" #include "flang/Semantics/tools.h"
#include <algorithm> #include <algorithm>
namespace Fortran::semantics { namespace Fortran::semantics {
// Use when clause falls under 'struct OmpClause' in 'parse-tree.h'. // Use when clause falls under 'struct OmpClause' in 'parse-tree.h'.
#define CHECK_SIMPLE_CLAUSE(X, Y) \ #define CHECK_SIMPLE_CLAUSE(X, Y) \
void OmpStructureChecker::Enter(const parser::OmpClause::X &) { \ void OmpStructureChecker::Enter(const parser::OmpClause::X &) { \
▲ Show 20 Lines • Show All 256 Lines • ▼ Show 20 Lines PushContextAndClauseSets(
dir.source, llvm::omp::Directive::OMPD_end_workshare); dir.source, llvm::omp::Directive::OMPD_end_workshare);
break; break;
default: default:
// no clauses are allowed // no clauses are allowed
break; break;
} }
} }
void OmpStructureChecker::Enter(const parser::OpenMPAtomicConstruct &x) {
std::visit(
common::visitors{
[&](const auto &someAtomicConstruct) {
const auto &dir{std::get<parser::Verbatim>(someAtomicConstruct.t)};
PushContextAndClauseSets(
dir.source, llvm::omp::Directive::OMPD_atomic);
},
},
x.u);
}
void OmpStructureChecker::Leave(const parser::OpenMPAtomicConstruct &) {
dirContext_.pop_back();
}
// Clauses // Clauses
// Mainly categorized as // Mainly categorized as
// 1. Checks on 'OmpClauseList' from 'parse-tree.h'. // 1. Checks on 'OmpClauseList' from 'parse-tree.h'.
// 2. Checks on clauses which fall under 'struct OmpClause' from parse-tree.h. // 2. Checks on clauses which fall under 'struct OmpClause' from parse-tree.h.
// 3. Checks on clauses which are not in 'struct OmpClause' from parse-tree.h. // 3. Checks on clauses which are not in 'struct OmpClause' from parse-tree.h.
void OmpStructureChecker::Leave(const parser::OmpClauseList &) { void OmpStructureChecker::Leave(const parser::OmpClauseList &) {
// 2.7 Loop Construct Restriction // 2.7 Loop Construct Restriction
▲ Show 20 Lines • Show All 103 Lines • ▼ Show 20 Lines
CHECK_SIMPLE_CLAUSE(Uniform, OMPC_uniform) CHECK_SIMPLE_CLAUSE(Uniform, OMPC_uniform)
CHECK_SIMPLE_CLAUSE(Untied, OMPC_untied) CHECK_SIMPLE_CLAUSE(Untied, OMPC_untied)
CHECK_SIMPLE_CLAUSE(UseDevicePtr, OMPC_use_device_ptr) CHECK_SIMPLE_CLAUSE(UseDevicePtr, OMPC_use_device_ptr)
CHECK_SIMPLE_CLAUSE(AcqRel, OMPC_acq_rel) CHECK_SIMPLE_CLAUSE(AcqRel, OMPC_acq_rel)
CHECK_SIMPLE_CLAUSE(Acquire, OMPC_acquire) CHECK_SIMPLE_CLAUSE(Acquire, OMPC_acquire)
CHECK_SIMPLE_CLAUSE(SeqCst, OMPC_seq_cst) CHECK_SIMPLE_CLAUSE(SeqCst, OMPC_seq_cst)
CHECK_SIMPLE_CLAUSE(Release, OMPC_release) CHECK_SIMPLE_CLAUSE(Release, OMPC_release)
CHECK_SIMPLE_CLAUSE(Relaxed, OMPC_relaxed) CHECK_SIMPLE_CLAUSE(Relaxed, OMPC_relaxed)
CHECK_SIMPLE_CLAUSE(Hint, OMPC_hint)
CHECK_REQ_SCALAR_INT_CLAUSE(Allocator, OMPC_allocator) CHECK_REQ_SCALAR_INT_CLAUSE(Allocator, OMPC_allocator)
CHECK_REQ_SCALAR_INT_CLAUSE(Grainsize, OMPC_grainsize) CHECK_REQ_SCALAR_INT_CLAUSE(Grainsize, OMPC_grainsize)
CHECK_REQ_SCALAR_INT_CLAUSE(NumTasks, OMPC_num_tasks) CHECK_REQ_SCALAR_INT_CLAUSE(NumTasks, OMPC_num_tasks)
CHECK_REQ_SCALAR_INT_CLAUSE(NumTeams, OMPC_num_teams) CHECK_REQ_SCALAR_INT_CLAUSE(NumTeams, OMPC_num_teams)
CHECK_REQ_SCALAR_INT_CLAUSE(NumThreads, OMPC_num_threads) CHECK_REQ_SCALAR_INT_CLAUSE(NumThreads, OMPC_num_threads)
CHECK_REQ_SCALAR_INT_CLAUSE(Priority, OMPC_priority) CHECK_REQ_SCALAR_INT_CLAUSE(Priority, OMPC_priority)
CHECK_REQ_SCALAR_INT_CLAUSE(ThreadLimit, OMPC_thread_limit) CHECK_REQ_SCALAR_INT_CLAUSE(ThreadLimit, OMPC_thread_limit)
▲ Show 20 Lines • Show All 53 Lines • ▼ Show 20 Lines
} }
// Following clauses have a seperate node in parse-tree.h. // Following clauses have a seperate node in parse-tree.h.
CHECK_SIMPLE_PARSER_CLAUSE(OmpAllocateClause, OMPC_allocate) CHECK_SIMPLE_PARSER_CLAUSE(OmpAllocateClause, OMPC_allocate)
CHECK_SIMPLE_PARSER_CLAUSE(OmpDefaultClause, OMPC_default) CHECK_SIMPLE_PARSER_CLAUSE(OmpDefaultClause, OMPC_default)
CHECK_SIMPLE_PARSER_CLAUSE(OmpDistScheduleClause, OMPC_dist_schedule) CHECK_SIMPLE_PARSER_CLAUSE(OmpDistScheduleClause, OMPC_dist_schedule)
CHECK_SIMPLE_PARSER_CLAUSE(OmpNowait, OMPC_nowait) CHECK_SIMPLE_PARSER_CLAUSE(OmpNowait, OMPC_nowait)
CHECK_SIMPLE_PARSER_CLAUSE(OmpProcBindClause, OMPC_proc_bind) CHECK_SIMPLE_PARSER_CLAUSE(OmpProcBindClause, OMPC_proc_bind)
CHECK_SIMPLE_PARSER_CLAUSE(OmpReductionClause, OMPC_reduction) CHECK_SIMPLE_PARSER_CLAUSE(OmpReductionClause, OMPC_reduction)
// Atomic-clause
CHECK_SIMPLE_PARSER_CLAUSE(OmpAtomicRead, OMPC_read)
CHECK_SIMPLE_PARSER_CLAUSE(OmpAtomicWrite, OMPC_write)
CHECK_SIMPLE_PARSER_CLAUSE(OmpAtomicUpdate, OMPC_update)
CHECK_SIMPLE_PARSER_CLAUSE(OmpAtomicCapture, OMPC_capture)
void OmpStructureChecker::Leave(const parser::OmpAtomicRead &) {
CheckNotAllowedIfClause(llvm::omp::Clause::OMPC_read,
{llvm::omp::Clause::OMPC_release, llvm::omp::Clause::OMPC_acq_rel});
}
void OmpStructureChecker::Leave(const parser::OmpAtomicWrite &) {
CheckNotAllowedIfClause(llvm::omp::Clause::OMPC_write,
{llvm::omp::Clause::OMPC_acquire, llvm::omp::Clause::OMPC_acq_rel});
}
void OmpStructureChecker::Leave(const parser::OmpAtomicUpdate &) {
CheckNotAllowedIfClause(llvm::omp::Clause::OMPC_update,
{llvm::omp::Clause::OMPC_acquire, llvm::omp::Clause::OMPC_acq_rel});
}
// OmpAtomic node represents atomic directive without atomic-clause.
clementvalUnsubmitted
Done
ReplyInline Actions

Why is there a specific check for this? If it is not allowed it should not be in OMP.td and then catch be CheckAllowed in enter acquire.

clementval: Why is there a specific check for this? If it is not allowed it should not be in `OMP.td` and…
sameeranjoshiAuthorUnsubmitted
Done
ReplyInline Actions

Standard says:
If atomic-clause is update or not present then memory-order-clause must not be acq_rel or acquire.

I couldn't use CheckNotAllowedIfClause here reason being there is no OMPC_atomic.
I see OMPC_atomic_default_mem_order but I doubt it's the same what I was trying to find.

also aquire acq_rel are not allowed only here they might be allowed somewhere else, hence changing them in OMP.td would affect other atomic directives as well.

sameeranjoshi: Standard says: `If atomic-clause is update or not present then memory-order-clause must not be…
clementvalUnsubmitted
Done
ReplyInline Actions

Then the check for update is missing? and it will fail for atomic read even it should be allowed?

clementval: Then the check for update is missing? and it will fail for atomic read even it should be…
sameeranjoshiAuthorUnsubmitted
Done
ReplyInline Actions

Then the check for update is missing?

umm... update check was added and I see that we have
test with comment ! 2.17.7.6 which works well due to CheckNotAllowedIfClause which fits for llvm::omp::Clause::OMPC_update

I might not be still clear on how to leverage TableGen for this particular test case.
I am happy to use TableGen wherever possible.
I even tried to add OMPC_Atomic but that's not helping.

sameeranjoshi: > Then the check for update is missing? umm... update check was added and I see that we have…
sameeranjoshiAuthorUnsubmitted
Done
ReplyInline Actions

Why is there a specific check for this?

Due to missing clause for ? inside below function which isn't in OMP.td.

CheckNotAllowedIfClause( ? ,
      {llvm::omp::Clause::OMPC_acquire, llvm::omp::Clause::OMPC_acq_rel})

Reason being that there is nothing mentioned inside TableGen.

If it is not allowed it should not be in OMP.td and then catch be CheckAllowed in enter acquire.

If we remove it from OMP.td that would be not found for other atomic clauses where it was expected to work.

sameeranjoshi: > Why is there a specific check for this? Due to missing clause for `?` inside below function…
// atomic-clause - READ,WRITE,UPDATE,CAPTURE.
clementvalUnsubmitted
Done
ReplyInline Actions

Where do you check that it is only for update?

clementval: Where do you check that it is only for update?
sameeranjoshiAuthorUnsubmitted
Done
ReplyInline Actions

I check separately for both
If atomic-clause is update or not present then memory-order-clause must not be acq_rel or acquire.
break into below
restriction:

If atomic-clause is update then memory-order-clause must not be acq_rel or acquire.

in
void OmpStructureChecker::Leave(const parser::OmpAtomicUpdate &)
&

If atomic-clause is not present then memory-order-clause must not be acq_rel or acquire.

in this Leave.

sameeranjoshi: I check separately for both `If atomic-clause is update or not present then memory-order-clause…
clementvalUnsubmitted
Done
ReplyInline Actions

Ok I see. The OmpAtomic is the atomic construct without clause? If so, maybe adding small comment here to precise this would be nice.

clementval: Ok I see. The `OmpAtomic` is the atomic construct without clause? If so, maybe adding small…
sameeranjoshiAuthorUnsubmitted
Done
ReplyInline Actions

Ok I see. The OmpAtomic is the atomic construct without clause? If so, maybe adding small comment here to precise this would be nice.

To be specific it's not a general clause but called as 'atomic-clause' in standard.
Which has values read/write/update/capture.

To answer the question - yes it doesn't have 'atomic-clause'.

!$omp atomic [clause[[,] clause] ... ] <no atomic clause here>
     update-statement
[!$omp end atomic]
sameeranjoshi: > Ok I see. The `OmpAtomic` is the atomic construct without clause? If so, maybe adding small…
clementvalUnsubmitted
Not Done
ReplyInline Actions

Yeah I get it. Since other "with" clause are represented by a node as well like OmpAtomicUpdate I still think it would make sense to have a small comment here to mentioned that the OmpAtomic node represent and atomic directive without atomic-clause

clementval: Yeah I get it. Since other "with" clause are represented by a node as well like…
void OmpStructureChecker::Leave(const parser::OmpAtomic &) {
if (const auto *clause{FindClause(llvm::omp::Clause::OMPC_acquire)}) {
context_.Say(clause->source,
clementvalUnsubmitted
Done
ReplyInline Actions

Same here. This should be catch be the generic code and the TableGen map.

clementval: Same here. This should be catch be the generic code and the TableGen map.
"Clause ACQUIRE is not allowed on the ATOMIC directive"_err_en_US);
}
if (const auto *clause{FindClause(llvm::omp::Clause::OMPC_acq_rel)}) {
context_.Say(clause->source,
"Clause ACQ_REL is not allowed on the ATOMIC directive"_err_en_US);
}
}
// Restrictions specific to each clause are implemented apart from the // Restrictions specific to each clause are implemented apart from the
// generalized restrictions. // generalized restrictions.
void OmpStructureChecker::Enter(const parser::OmpAlignedClause &x) { void OmpStructureChecker::Enter(const parser::OmpAlignedClause &x) {
CheckAllowed(llvm::omp::Clause::OMPC_aligned); CheckAllowed(llvm::omp::Clause::OMPC_aligned);
if (const auto &expr{ if (const auto &expr{
std::get<std::optional<parser::ScalarIntConstantExpr>>(x.t)}) { std::get<std::optional<parser::ScalarIntConstantExpr>>(x.t)}) {
RequiresConstantPositiveParameter(llvm::omp::Clause::OMPC_aligned, *expr); RequiresConstantPositiveParameter(llvm::omp::Clause::OMPC_aligned, *expr);
▲ Show 20 Lines • Show All 271 Lines • Show Last 20 Lines