Meta directive runtime support

[abidmalikwaterloo]

Summary

The patch extends the current implementation
for runtime support. The current patch build AST
of the when clauses in sorted order using the
context selector score

[jdoerfert]

This contains a lot of unrelated changes, leftover comments, etc. As you clean up the code, please also include tests. Clang format the patch, go over the new code and make sure you follow the coding style. Remove unneeded parts (e.g., a map into which you simply push objects then take them out to push them into a vector).

Also, have you seen D120573?

[abidmalikwaterloo]

I quickly went through the D120573. There are many overlapping.

[abidmalikwaterloo]

Is it okay if I can put comments on the patch?

[jdoerfert]

In D122255#3400636, @abidmalikwaterloo wrote:

Is it okay if I can put comments on the patch?

Not sure what you mean but feel free.

[abidmalikwaterloo]

would it make sense to submit separate patches for when (OMPC_when) and otherwise (OMPC_otherwise) clauses? The D120573 also has OMPC_when support.

[abidmalikwaterloo]

There should be an update to D122255. It should not create the new one.D123598
--->
I did the following :
arc patch D122255
"did something/cleaning"
git commit -a
arc diff
---->

[abidmalikwaterloo]

I update it but it created a new patch D123598.
I marked it Abandon. Not sure how to delete it.
I am submitting it again after cleaning the codes
and adding some basic tests.

[abidmalikwaterloo]

Added tests

[abidmalikwaterloo]

The last update removed all previous updates. Can you point me to the correct way to do it?
I did the following :
arc patch D122255
"did something/cleaning"
git commit -a --amend
arc diff

[abidmalikwaterloo]

Cleaned the code and added tests.

[abidmalikwaterloo]

Should I update the patch? I am waiting for quick feedback on the concept of handling conditions.

[ABataev]

clang/include/clang/AST/OpenMPClause.h

8859 ↗

(On Diff #423305)

condition?

8893–8894 ↗

(On Diff #423305)

make it private

8900 ↗

(On Diff #423305)

const

8905 ↗

(On Diff #423305)

const

clang/include/clang/AST/StmtOpenMP.h

5479 ↗

(On Diff #423305)

Remove this change

clang/include/clang/Basic/DiagnosticSemaKinds.td

10852 ↗

(On Diff #423305)

We do not use explamations in messages, better to have something only single default ...

clang/include/clang/Sema/Sema.h

10688 ↗

(On Diff #423305)

Comment for this new function

11138 ↗

(On Diff #423305)

formatting

clang/lib/AST/OpenMPClause.cpp

1614–1617 ↗

(On Diff #423305)

Is this correct? Just default( is expected to be printed?

clang/lib/AST/StmtPrinter.cpp

658 ↗

(On Diff #423305)

Do not insert empty line here

661 ↗

(On Diff #423305)

formatting

665 ↗

(On Diff #423305)

use isa

667 ↗

(On Diff #423305)

Camel naming style expceted

668 ↗

(On Diff #423305)

formatting, use nullptr instead of NULL (or just C)

669 ↗

(On Diff #423305)

Formatting

clang/lib/Parse/ParseOpenMP.cpp

2433 ↗

(On Diff #423305)

What is this?

2474 ↗

(On Diff #423305)

Restore original code here

2498–2506 ↗

(On Diff #423305)

Remove this extra stuff

2546 ↗

(On Diff #423305)

What A stands after, what does it mean?

[abidmalikwaterloo]

clang/include/clang/AST/OpenMPClause.h

8893–8894 ↗

(On Diff #423305)

Any reason behind this? I went through other classes, this function is public in some and private in some.

[ABataev]

clang/include/clang/AST/OpenMPClause.h

8893–8894 ↗

(On Diff #423305)

The AST is immutable, such functions are/must be used in very rare places.

[abidmalikwaterloo]

Updated and clean the code based on the comments from
the reviewer!

[abidmalikwaterloo]

Ping!

clang/lib/AST/OpenMPClause.cpp

1614–1617 ↗

(On Diff #423305)

This function is called from StmtPrinter::PrintOMPExecutableDirective in StmtPrinter.CPP. The enclosed Directive and the second matching brace will be printed. See line # 670 in StmtPrinter.CPP

clang/lib/Parse/ParseOpenMP.cpp

2433 ↗

(On Diff #423305)

comment removed

[jdoerfert]

clang/include/clang/Basic/DiagnosticSemaKinds.td

10852 ↗

(On Diff #423305)

"allowed in" what? Also, there is no test for this, or is there?

clang/lib/AST/OpenMPClause.cpp

1614–1617 ↗

(On Diff #423305)

The problem is this doesn't make sense. For one, StmtPrinter calls this function conditionally, so you might end up with a lonely closing parenthesis. Also, why would we split the parenthesis into two places?

1679 ↗

(On Diff #442661)

I'm assuming we already have a printer for trait selectors, no? Doesn't OMPTraitInfo::print do this already and actually handle scores?

clang/lib/AST/StmtPrinter.cpp

667 ↗

(On Diff #442661)

clang/lib/Parse/ParseOpenMP.cpp

2474 ↗

(On Diff #423305)

There are plenty of formatting changes in this file and elsewhere. Please retore the original code or properly clang-format the patch.

2544 ↗

(On Diff #442661)

typo

2567 ↗

(On Diff #442661)

We don't use comments like these.

2607 ↗

(On Diff #442661)

This seems to be copied from somewhere. It is unclear why a metadirective needs to have an associated openmp region.

clang/lib/Sema/SemaOpenMP.cpp

4704 ↗

(On Diff #442661)

Still unrelated newlines.

7390 ↗

(On Diff #442661)

Code style, ArchMatch.

7439 ↗

(On Diff #442661)

Why does this perform partial trait matching? We should have code for this. Also, the logic for device_arch and vendor (which is most what there is), is not what we want. Reuse the existing matching logic instead.

clang/lib/Sema/SemaStmt.cpp

4795 ↗

(On Diff #442661)

Unrelated. Please go over the patch and avoid these in the future. We have so many comments that point out these things that now the comments make it hard to read/review.

clang/test/OpenMP/metadirective_ast_print_new_1.cpp

20 ↗

(On Diff #442661)

I doubt clang format handles the metadirective well but maybe format the code first without the pragma. This is hard to read. Also the other tests.

You only have ast print tests, what about the rest? Messages (errors/warnings) and codegen?

llvm/lib/Frontend/OpenMP/OMPContext.cpp

378 ↗

(On Diff #442661)

This does not sort scores properly and for some reason uses a Map as a intermediate storage.
Instead, please combine this with the function below, which already has a proper score comparator.

[abidmalikwaterloo]

clang/include/clang/Basic/DiagnosticSemaKinds.td

10852 ↗

(On Diff #423305)

It should be "Only one default clause is allowed.

clang/lib/AST/OpenMPClause.cpp

1679 ↗

(On Diff #442661)

Looked into the function. OMPTraitInfo::print can be used. The function needs to be extended as well to take the other traits as well.

clang/lib/Sema/SemaOpenMP.cpp

7439 ↗

(On Diff #442661)

Ok. What do you mean by existing matching logic?

clang/lib/Sema/SemaStmt.cpp

4795 ↗

(On Diff #442661)

An accidental tap. Removed

[abidmalikwaterloo]

clang/include/clang/AST/RecursiveASTVisitor.h

3144 ↗

(On Diff #442661)

If it is not the user's condition or score then what will happen?

clang/lib/Sema/SemaOpenMP.cpp

7439 ↗

(On Diff #442661)

@jdoerfert I agree that the implementation is incomplete in terms of trait matching. It can be completed. However, I am not clear about your comments about the existing matching logic. I checked OMPContext.CPP and other files. There are functions that can be used to match the traits. But, I could not find any existing logic that can be used here.

[jdoerfert]

clang/lib/AST/OpenMPClause.cpp

1679 ↗

(On Diff #442661)

The function needs to be extended as well to take the other traits as well.

What traits are not handled there?

clang/lib/Sema/SemaOpenMP.cpp

7439 ↗

(On Diff #442661)

I mean getBestVariantMatchForContext and similar functions in llvm/lib/Frontend/OpenMP/OMPContext.cpp.

[abidmalikwaterloo]

I will work on the patch in parts. I am planning to submit taking care of comments except for comments for SemaOpenMP and code generation.

clang/lib/AST/OpenMPClause.cpp

1679 ↗

(On Diff #442661)

No. It handles everything in terms of printing. I have updated the code. Will upload it as well.

clang/lib/Parse/ParseOpenMP.cpp

2607 ↗

(On Diff #442661)

correcrted.

clang/lib/Sema/SemaOpenMP.cpp

7439 ↗

(On Diff #442661)

OK.

[abidmalikwaterloo]

Updated the PrintStmt.cpp according to the comments

[ABataev]

clang/lib/AST/OpenMPClause.cpp

19 ↗

(On Diff #461661)

Remove empty comment

1620–1621 ↗

(On Diff #461661)

if (Stmt *S = Node->getDirective())

2411–2419 ↗

(On Diff #461661)

Remove those empty new lines

[abidmalikwaterloo]

Clean the formatting comments. The patch is out of attention
for a while. Still need to work on the context selector part
as a next step.

[abidmalikwaterloo]

Rebase the patch to origin/main with all updates. Made changes
to make it buildable using the new changes.
(WIP).

clang/test/OpenMP/metadirective_ast_print_new1.c

 // RUN: %clang_cc1 -verify  -fopenmp  -ast-print %s -o - | FileCheck %s
 // expected-no-diagnostics
 void bar(){
         int i=0;
 }
 
 void myfoo(void){
 
         int N = 13;
         int b,n;
         int a[100];
 
         #pragma omp metadirective when(user={condition(N>10)}:  target teams ) default(parallel for)
                 for (int i = 0; i < N; i++)
                 bar();
 
 }
 
 // CHECK: void bar()
 // CHECK: #pragma omp metadirective when(user={condition(N > 10)}: target teams) default(parallel for)

clang/test/OpenMP/metadirective_ast_print_new2.c

 // RUN: %clang_cc1 -verify  -fopenmp  -ast-print %s -o - | FileCheck %s
 // expected-no-diagnostics
 
 void bar(){
   int i=0;  
 }
 
 void myfoo(void){
 
   int N = 13;
   int b,n;
   int a[100];
 
   
   #pragma omp  metadirective when (user = {condition(N>10)}: target  teams distribute parallel for ) \
           when (user = {condition(N==10)}: parallel for )\
           when (user = {condition(N==13)}: parallel for simd) \
           when ( device={arch("arm")}:   target teams num_teams(512) thread_limit(32))\
           when ( device={arch("nvptx")}: target teams num_teams(512) thread_limit(32))\
           default ( parallel for)\
 
   {    for (int i = 0; i < N; i++)
     bar();
   }
 }
 
 // CHECK: bar()
 // CHECK: myfoo
 // CHECK: #pragma omp metadirective when(user={condition(N > 10)}: target teams distribute parallel for) when(user={condition(N == 13)}: parallel for simd) when(device={arch(nvptx)}: target teams)

clang/test/OpenMP/metadirective_ast_print_new3.c

 // RUN: %clang_cc1 -verify  -fopenmp  -ast-print %s -o - | FileCheck %s
 // expected-no-diagnostics
 
 int main() {
   int N = 15;
 #pragma omp metadirective when(user = {condition(N > 10)} : parallel for)\
         default(target teams) 
   for (int i = 0; i < N; i++)
     ;
 
 
 #pragma omp metadirective when(device = {arch("nvptx64")}, user = {condition(N >= 100)} : parallel for)\
           default(target parallel for)
   for (int i = 0; i < N; i++)
     ;
   return 0;
 }
 
 
 
 // CHECK: #pragma omp metadirective when(user={condition(N > 10)}: parallel for) default(target teams)
 // CHECK: #pragma omp metadirective when(device={arch(nvptx64)}, user={condition(N >= 100)}: parallel for) default(target parallel for)