This is an archive of the discontinued LLVM Phabricator instance.

[flang][OpenMP] Add semantic check for target nesting
ClosedPublic

Authored by peixin on Jul 16 2021, 10:19 AM.

Details

Summary

This patch implements the following check for TARGET construct:

OpenMP Version 5.0 Target construct restriction: If a target update,
target data, target enter data, or target exit data construct is
encountered during execution of a target region, the behavior is
unspecified.

Also add one test case for the check.

Diff Detail

Event Timeline

peixin created this revision.Jul 16 2021, 10:19 AM
peixin requested review of this revision.Jul 16 2021, 10:19 AM
clementval requested changes to this revision.Jul 16 2021, 10:32 AM
clementval added inline comments.
flang/lib/Semantics/check-omp-structure.cpp
291

It would be better to have a generic IsNestedIn function where you can pass the directive you want to check for. We don't want to have a counter for each construct.

518

The standard says it is unspecified but not really restricted.

669

As mentioned above you should use the current _dirContext stack with a generic function.

736

Same here. Please use generic mechanism.

flang/test/Semantics/omp-target01.f90
2

You don't check if your restriction actually work here.

This revision now requires changes to proceed.Jul 16 2021, 10:32 AM
peixin updated this revision to Diff 359515.Jul 16 2021, 7:17 PM

@clementval Thanks for the comments. Upload the following update:

  1. I am reminded that the counter is not necessary since the if branch dirContext_.size() >= 1 already checks the directive nested. Delete the counter usage and checking GetContext().directive == llvm::omp::Directive::OMPD_target inside dirContext_.size() >= 1 check is enough.
  2. Add the error check in test case.
  3. There is no this error info before, or at lease I did not find in f18 openmp semantic check. I test gfortran 9.3.0 and it gives the following warning:
   15 |   !$omp target update from(arrayA) to(arrayB)
Warning: ‘target update’ construct inside of ‘target’ region
   23 |   !$omp target data map(to: a)
Warning: ‘target data’ construct inside of ‘target’ region
   33 |   !$omp target enter data map(alloc:B)
Warning: ‘target enter data’ construct inside of ‘target’ region
   38 |   !$omp target exit data map(delete:B)
Warning: ‘target exit data’ construct inside of ‘target’ region

I also generate one similar c code to check target data directive according to OpenMP API Examples Version 5.0.1 as follows:

extern void init(float*, float*, int);
extern void output(float*, int);
void vec_mult(float *p, float *v1, float *v2, int N)
{
  int i;
  init(v1, v2, N);
  #pragma omp target
  #pragma omp target data map(to: v1[0:N], v2[:N]) map(from: p[0:N])
  {
    for (i=0; i<N; i++)
      p[i] = v1[i] * v2[i];
  }
  output(p, N);
}

Tested using gcc 9.3.0 and current clang and the results are as follows:

$ clang -fopenmp temp.c 
temp.c:8:3: error: region cannot be nested inside 'target' region
  #pragma omp target data map(to: v1[0:N], v2[:N]) map(from: p[0:N])
  ^
1 error generated.
$ gcc -fopenmp temp.c 
temp.c: In function ‘vec_mult’:
temp.c:8:11: warning: ‘target data’ construct inside of ‘target’ region
    8 |   #pragma omp target data map(to: v1[0:N], v2[:N]) map(from: p[0:N])
      |           ^~~

Then I use the similar error info as clang to keep the consistence.

peixin updated this revision to Diff 359678.Jul 19 2021, 12:11 AM

Set the flag eligibleTARGET as true by default and print the error when it is false.

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

Is the restriction for closely nested or does it apply to any kind of nesting? For e.g is the following allowed and will the test catch this?

-> target
-> -> parallel
-> -> -> target data

518

Same concern as @clementval. Should this be a warning or an error?

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

Nit: I think it will be better to rename this as CheckTargetNest.

peixin updated this revision to Diff 360040.Jul 20 2021, 12:55 AM
peixin set the repository for this revision to rG LLVM Github Monorepo.

@kiranchandramohan Thanks for the review.

The restriction applies to any kind of nesting. Add the test scenario of parallel directive between target and target update, which is not covered in previous design. Fixed the design to cover this scenario. Also rename the function name.

The error reporting is truly not completely reasonable. Change it into warning report and also fix the error info by replacing cannot by should not. Is this OK with you? @kiranchandramohan @clementval

peixin added inline comments.Jul 21 2021, 1:29 AM
flang/lib/Semantics/check-omp-structure.cpp
291

I am thinking about if we can replace this for loop search with the counter marked for target nesting since the for loop is executed for each openmp construct check, which is not efficient. With the design in https://reviews.llvm.org/D106335, I only need add one more element in the enum definition of directiveNestType. What do you think?

kiranchandramohan requested changes to this revision.Jul 28 2021, 2:49 PM

Have some suggestions.

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

That sounds good to me.

I think ideally, you can do the following.
On visiting a target_data, target_update, target_enter_data or target_exit_data directive check if it is nested in a target region using GetDirectiveNest and report a warning if it is nested.

483

The name of the variable does not match its intention. Like these are the ones that cause a warning if true, right?

521

Can the warning be changed to what the standard says, that the "behaviour is unspecified"?

This revision now requires changes to proceed.Jul 28 2021, 2:49 PM
peixin updated this revision to Diff 362725.Jul 29 2021, 5:16 AM
peixin added a reviewer: Chuanfeng.

I will rebase this patch or https://reviews.llvm.org/D106335 after either of them is merged.

@kiranchandramohan Thanks for the comments.

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

Thanks for the suggestion. Fixed now.

483

Yes. Sorry for that. Reversed the value to match its intention.

521

Replaced the warning message with what the standard says according to your suggestion.

kiranchandramohan requested changes to this revision.Jul 30 2021, 8:21 AM

Looks OK. I have a question and a suggestion. Please make the change if you agree otherwise let me know.

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

I was suggesting something like the following to avoid double traversal of OpenMPBlockConstruct. We would be visiting these nodes anyway during regular traversal.
Have you considered this? Feel free to dissent.

void OmpStructureChecker::Enter(const parser::OpenMPBlockConstruct &x) {
  const auto &beginBlockDir{std::get<parser::OmpBeginBlockDirective>(c.t)};
  const auto &beginDir{std::get<parser::OmpBlockDirective>(beginBlockDir.t)};
   if (GetDirectiveNest(TargetNest) > 0) {
     if (beginDir.v == llvm::omp::Directive::OMPD_target_data) {
       //print warning
     }
}
483

Nit:Would eligibleTargetDir be better?

This revision now requires changes to proceed.Jul 30 2021, 8:21 AM

@kiranchandramohan Thanks for the review.

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

Actually, my first implementation is like what you suggest. Arnamoy suggests the current implementation and I argee with him. Merging the semantic checks for target_data and other three directives in one function seems easier to read and understand. In addition, implementing the semantic check of the four directives nested inside target in one fuction seems more reasonable since they are in one restriction. This is one time-readability tradeoff. I think the double traversal does not take too much time. What do you think?

483

elibleTargetDir does not match its intention. Would ineligibleTargetDir be better?

LGTM. One minor comment.

@clementval do you have further comments?

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

OK.

483

Yes, I would prefer ineligibleTargetDir.

This revision now requires review to proceed.Jul 31 2021, 1:24 AM
peixin updated this revision to Diff 363329.Jul 31 2021, 7:52 PM

@kiranchandramohan Thanks for the review.
Replaced the variable name directive with ineligibleTargetDir.

peixin updated this revision to Diff 364366.Aug 5 2021, 12:06 AM

Fix the clang-format warning.

clementval accepted this revision.Aug 12 2021, 7:13 PM

LGTM. Sorry I was kinda busy the last few days and forgot to review the changes. Please go ahead and land the patch.

This revision is now accepted and ready to land.Aug 12 2021, 7:13 PM
peixin updated this revision to Diff 366401.Aug 13 2021, 11:30 PM

Rebase this patch.

peixin updated this revision to Diff 366431.Aug 14 2021, 4:47 AM

Fix the clang format warning.

This revision was automatically updated to reflect the committed changes.