This is an archive of the discontinued LLVM Phabricator instance.

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

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



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

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.

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.


The standard says it is unspecified but not really restricted.


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


Same here. Please use generic mechanism.

1 ↗(On Diff #359375)

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.


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


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


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

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, 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.


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.


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


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 after either of them is merged.

@kiranchandramohan Thanks for the comments.


Thanks for the suggestion. Fixed now.


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


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.


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

Nit:Would eligibleTargetDir be better?

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

@kiranchandramohan Thanks for the review.


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?


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

LGTM. One minor comment.

@clementval do you have further comments?




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.