Page MenuHomePhabricator

[OPENMP] Add support for nested 'declare target' directives
ClosedPublic

Authored by patricklyster on Aug 28 2018, 1:09 PM.

Diff Detail

Event Timeline

patricklyster created this revision.Aug 28 2018, 1:09 PM
This revision is now accepted and ready to land.Aug 28 2018, 1:10 PM

Is there a way to tell if the header files have matching omp declare target/omp end declare target.
The reason is if one of the header files is missing a matching omp end declare target all files which include it will end up having everything marked with declare target

Is there a way to tell if the header files have matching omp declare target/omp end declare target.
The reason is if one of the header files is missing a matching omp end declare target all files which include it will end up having everything marked with declare target

Are you suggesting to throw a warning/error at the end of the header file should there be a hanging omp declare target? Wouldn't this still be the same issue with each file that includes it still getting the same warning/error?

There is a corner case when the header file is missing the end declare target and the file that includes it has an extra end declare target. In this case no error will be thrown and all code in between the directives will be associated with the declare target. However, I don't know if this is something worth checking for.

We should just go with generating an error if the DeclareTargetNestingLevel is not 0 at the end of compilation unit.
Hard to detect if user accidentally forgot to have end declare in header file and had it in the include file or it was intentional.

We should just go with generating an error if the DeclareTargetNestingLevel is not 0 at the end of compilation unit.
Hard to detect if user accidentally forgot to have end declare in header file and had it in the include file or it was intentional.

That will effectively forbid the legacy approach of doing

#pragma omp declare target
#include <...>
#pragma omp end declare target

as in the test because DeclareTargetNestingLevel will be 1 throughout the header file. I think that's still relevant today, so the condition should be "has the same value as when entering this file".

We should just go with generating an error if the DeclareTargetNestingLevel is not 0 at the end of compilation unit.
Hard to detect if user accidentally forgot to have end declare in header file and had it in the include file or it was intentional.

This is the current behaviour

I did not see the code where check is done if Nestingdepth is 0 at end of compilation.

patricklyster added a comment.EditedAug 30 2018, 11:43 AM

I did not see the code where check is done if Nestingdepth is 0 at end of compilation.

Sorry I should clarify. You will get an error if you have an unmatched #pragma omp declare target once you reach the end of the compilation unit (not the header file) because execution will exit the while loop in ParseOpenMP.cpp:774 when it encounters a tok::eof and fall into the else statement on ParseOpenMP.cpp:812 which emits the diagnostic. So it is not explicitly checking if DeclareTargetNestingLevel > 0 but you will see an appropriate diagnostic anyways.

Previous implementation only supported immediately nested declare targets such as below:

#pragma omp declare target
  #pragma omp declare target
    int foo();
  #pragma omp end declare target
  int bar();
#pragma omp end declare target

Update allows for nesting in this format:

#pragma omp declare target
  int foo();
  #pragma omp declare target
    int bar();
  #pragma omp end declare target
#pragma omp end declare target
This revision was automatically updated to reflect the committed changes.