This is an archive of the discontinued LLVM Phabricator instance.

[OPENMP51]Initial support for the interop directive
ClosedPublic

Authored by mikerice on Mar 12 2021, 2:40 PM.

Details

Summary

Adds basic parsing/sema/serialization support for the #pragma omp interop directive.

Nothing too unusual here. Some points to consider:

  1. A 'destroy' clause already exists that differs from 'destroy' used on 'interop' that uses a variable. This results in a nullptr for the variable when that type of 'destroy'.
  1. There wasn't a clearly good way to represent the 'init' clause. It has one Expr for the variable followed by an optional list of int/string Exprs so requires a TrailingExprList. I used an OMPVarListClause since it provides the functionality of trailing Exprs it doesn't really contain a 'varlist'. It didn't seem worth it to roll my own with almost the exact same functionality or rename OMPVarListClause but am open to ideas.
  1. I didn't do any validation of the preference-list yet. Since implementation-defined values are allowed and different implementations might only support some of the known values we might want to give a) warning for known but unsupported values and/or b) warning for completely unknown values. Or maybe warn anytime an implementation doesn't support a value. Should these value be hardcoded in the compiler at all? Or just determined from the headers?

First patch includes 'init' clause. Separately patches for 'use' and 'destroy' will follow.

Diff Detail

Event Timeline

mikerice created this revision.Mar 12 2021, 2:40 PM
mikerice requested review of this revision.Mar 12 2021, 2:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 12 2021, 2:40 PM
ABataev added inline comments.Mar 15 2021, 7:27 AM
clang/include/clang/AST/OpenMPClause.h
7491

I suggest splitting this patch anв keep just one clause. Other clauses should be added in follow-up patches.

7524–7527

This is not always correct. Need to check if this is the extended clause and return this child range only for the extended clause. For the ordinary one need to return an empty range.

clang/lib/AST/OpenMPClause.cpp
1526

llvm::copy

clang/lib/Parse/ParseOpenMP.cpp
2937–2939

Enclose in braces to keep uniform formatting

clang/lib/Sema/SemaOpenMP.cpp
14630–14631
else {
assert(isa<OMPDependClause>(C) && "...");
...
}
14706–14707

Braces

14709

Braces

mikerice marked 5 inline comments as done.Mar 15 2021, 9:35 AM

Thanks for the review.

clang/include/clang/AST/OpenMPClause.h
7491

Sorry I should have done that. Unfortunately the logic is somewhat shared among these three and breaking it up at this point would mean rewriting it without those interactions. I can do it if you really want me to.

clang/lib/Sema/SemaOpenMP.cpp
14630–14631

Sorry, I'm not following this comment. This is just doing something on 'init' and 'depend' clauses. Other clauses can exist on the directive and need to be skipped in the loop.

mikerice updated this revision to Diff 330696.Mar 15 2021, 9:37 AM

Addressed comments, fixed clang-formatting.

ABataev added inline comments.Mar 15 2021, 9:37 AM
clang/include/clang/AST/OpenMPClause.h
7491

Better to split, if possible.

clang/lib/Sema/SemaOpenMP.cpp
14630–14631

Ok, I see

ABataev added inline comments.Mar 15 2021, 10:04 AM
clang/include/clang/AST/OpenMPClause.h
7423–7424

Do we really need these params? I assume they are already part of Exprs though in Expr * form.

clang/lib/Parse/ParseOpenMP.cpp
3086–3088

Braces here too or remove braces from the if substatement.

3123–3126
if (!Tok.is(tok::comma))
  break;
ConsumeToken();
3160

append?

clang/lib/Sema/SemaOpenMP.cpp
14749

range-based loop:

for (const Expr *E : llvm::drop_begin(Exprs))
mikerice marked 4 inline comments as done.Mar 15 2021, 11:09 AM
mikerice added inline comments.
clang/include/clang/AST/OpenMPClause.h
7423–7424

The 'target' and 'targetsync' interop-types are not parsed as Exprs. Are you suggesting we should create Exprs for these?

7491

Should be possible. Is it better to start a new review or just update this one?

ABataev added inline comments.Mar 15 2021, 11:38 AM
clang/include/clang/AST/OpenMPClause.h
7491

Just update this one.

cchen added a subscriber: cchen.Mar 15 2021, 12:45 PM
mikerice updated this revision to Diff 330795.Mar 15 2021, 1:40 PM
mikerice edited the summary of this revision. (Show Details)

Split original patch. Now includes just the directive plus the 'init' clause.

mikerice updated this revision to Diff 330854.Mar 15 2021, 5:53 PM

Fixed clang-format.

It looks like adding the new clause breaks the flang build. Anyone know how to address that?

ld.lld: error: undefined symbol: Fortran::semantics::OmpStructureChecker::Enter(Fortran::parser::OmpClause::Init const&)

referenced by semantics.cpp:83 (/mnt/disks/ssd0/agent/llvm-project/flang/lib/Semantics/semantics.cpp:83)

semantics.cpp.o:
ABataev added inline comments.Mar 16 2021, 5:44 AM
clang/include/clang/AST/OpenMPClause.h
7423–7424

Oh, removed the wrong comment :(
I suggest splitting Exprs parameter and do not include interop variable there, pass this variable in a separate param, but still store it as part of Expr* list

7477

Beter to use std::next(Children.begin()) or Children.begin() + 1

clang/lib/AST/OpenMPClause.cpp
1782

auto->Expr

clang/lib/Sema/SemaOpenMP.cpp
14631

auto->OMPClause

14648

auto->OMPClause

14649

auto->OMPClauseKind

14653–14663

Better to use if (ClauseKind == OMPC_init) here

clang/lib/Sema/TreeTransform.h
9307

auto->Expr

clang/lib/Serialization/ASTReader.cpp
12141

i->I

clang/lib/Serialization/ASTWriter.cpp
6220
  1. auto->Expr

2> No need for braces

mikerice updated this revision to Diff 331093.Mar 16 2021, 1:30 PM
mikerice marked 9 inline comments as done.

Addressed review comments. Added line to flang to fix build error.

ABataev added inline comments.Mar 16 2021, 1:41 PM
clang/lib/Sema/SemaOpenMP.cpp
14647

Better to use SmallPtrSet

14711

I don't think you need to call getNonLValueExprType() here since InteropVarExpr is an expression already.

mikerice marked 2 inline comments as done.Mar 16 2021, 2:27 PM
mikerice added inline comments.
clang/lib/Sema/SemaOpenMP.cpp
14711

Just needs the unqualified type.

mikerice updated this revision to Diff 331108.Mar 16 2021, 2:28 PM
mikerice marked an inline comment as done.

Addressed review comments.

ABataev added inline comments.Mar 16 2021, 5:26 PM
clang/lib/Sema/SemaOpenMP.cpp
14711

What if init clause is applied to const qualified var? Is this allowed?

mikerice added inline comments.Mar 16 2021, 7:45 PM
clang/lib/Sema/SemaOpenMP.cpp
14711

No it isn't allowed, but that is checked below and 'VarType' is not used there, it uses the fully qualified type from the original expression.

This revision is now accepted and ready to land.Mar 17 2021, 3:39 AM
This revision was landed with ongoing or failed builds.Mar 17 2021, 9:42 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 17 2021, 9:42 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript