[Clang][OpenMP] Support for Code Generation of loop bind clause

[koops]

Summary

Support for Code Generation of "#pragma loop bind" clause.

1. bind(parallel)
2. bind(teams)
3. bind(thread)

[koops]

bind(thread) is not working at present. I have uploaded this patch to obtain feedback mainly on this.

[jdoerfert]

We need tests.

clang/lib/CodeGen/CGStmtOpenMP.cpp

7790 ↗

(On Diff #499780)

style, also below.

[ronlieb]

LIT Test ?

[ddpagan]

How is bind(thread) not working? Runtime issue?

[carlo.bertolli]

clang/lib/CodeGen/CGStmtOpenMP.cpp

7791 ↗

(On Diff #499780)

What if you have something like this:

void foo(..) {

#pragma omp loop
for (..) {
}

}

int main() {

#pragma omp target teams
{
  ...
  foo();
 }

#pragma omp target
 {
    #pragma omp parallel
    {
       foo();
    }

}

In the first invocation, loop is bound to teams. In the second, loop is bound to parallel.
This is a runtime condition.

I *believe* that not even OpenMP 6.0 TR1 allows us to decide at compile time if loop should be treated as worksharing or workdistribution....but I might be wrong.

Thanks!

[koops]

clang/lib/CodeGen/CGStmtOpenMP.cpp

7791 ↗

(On Diff #499780)

The loop directive does not have a bind clause. If you examine the current patch it takes care to preserve the old structure as is and the binding is done by default during runtime.

[carlo.bertolli]

clang/lib/CodeGen/CGStmtOpenMP.cpp

7791 ↗

(On Diff #499780)

Do you mean that this patch adds up to existing support - adding support for bind - and that existing support is already based on runtime support to decide on the case I mentioned?

[carlo.bertolli]

clang/lib/CodeGen/CGStmtOpenMP.cpp

7791 ↗

(On Diff #499780)

Disregard my comment: essentially, loop cannot be orphaned in the way I described.

[koops]

1. formatting
2. Adding lit test
3. Removing bind clause from the set of clauses passed during bind(parallel) to the OMPForDirective and bind(teams) to the OMPDistributeDirective.

[ABataev]

clang/lib/AST/StmtOpenMP.cpp

2362 ↗

(On Diff #505869)

We use tail allocation, this is definetely a mem leak.

clang/lib/CodeGen/CGStmtOpenMP.cpp

7815–7851 ↗

(On Diff #505869)

This is a bad place for this kind of operation here, it should be handled in Sema

[koops]

Moving the code to SemaOpenMP.cpp from CodeGen.

[koops]

Removing changes from :

1. clang/include/clang/AST/StmtOpenMP.h
2. clang/lib/AST/StmtOpenMP.cpp
3. clang/include/clang/Parse/Parser.h

These were useful when the code was in CodeGen to handle the bind clause.

[ABataev]

clang/lib/Sema/SemaOpenMP.cpp

6129

Remove this new line

6185–6193

1. You're overriding the directive kind here and do not restore it then. It may cause the compiler crash.
2. I think you need to here to create a new OpenMP region rather than overriding the existing one.

clang/test/OpenMP/loop_bind_codegen.cpp

2

Remove this

52–57

I think it should trigger the assert in setCurrentDirective

[koops]

clang/test/OpenMP/loop_bind_codegen.cpp

52–57

I did not see any crash at this point. Can you please give me other examples where I can see this crash?

[ABataev]

clang/include/clang/Basic/DiagnosticSemaKinds.td

9819

err_omp_...

clang/lib/Sema/SemaOpenMP.cpp

6150

ParentDirective

6171–6172

Remove extra parens

6177–6179

Remove extra braces

6185–6193

I see why you're doing this now. Would be good to see some semantic tests, like nesting of regions, use of unsupported clauses (e.g. linear on loop bind(teams)), etc.

6206–6212

Why need to drop bind clause here?

clang/test/OpenMP/loop_bind_codegen.cpp

52–57

Ah, you're overriding the loop directive kind.

[koops]

1. Adding semantic test clang/test/OpenMP/loop_bind_messages.cpp.
2. Changes suggested by Alexey.
3. >Why need to drop bind clause here? The new Directives to which loop directive is being mapped to, do not contain the bind clause. (a) omp loop bind(parallel) ----> omp for (b) omp loop bind(teams) -----> omp distribute (c) omp loop bind(thread) ------> omp simd

[ABataev]

In D144634#4272857, @koops wrote:

1. Adding semantic test clang/test/OpenMP/loop_bind_messages.cpp.
2. Changes suggested by Alexey.
3. >Why need to drop bind clause here? The new Directives to which loop directive is being mapped to, do not contain the bind clause. (a) omp loop bind(parallel) ----> omp for (b) omp loop bind(teams) -----> omp distribute (c) omp loop bind(thread) ------> omp simd

But why need to drop it? It makes processing more complex. The bind clause itself should not be a problem, no?

clang/lib/Sema/SemaOpenMP.cpp

644

NewDK

646

Add message to the assert.

clang/test/OpenMP/loop_bind_codegen.cpp

3–4

In other tests, this is used for testing with precompiled headers. You don't have this in your test runs currently.

9–14

Remove this commented code too

clang/test/OpenMP/loop_bind_messages.cpp

2–3

1. No need for this in semantic test

2, More tests required, nesting of regions, more tests for incompatible clauses.

[ABataev]

Also, add AST printing tests

[koops]

1. Taken care of some of the comments by Alexey.
2. Added extra tests of loop_bind_messages.cpp

However, generic_loop_messages.cpp & generic_loop_ast_print.cpp are present and provide a good coverage.

[ABataev]

In D144634#4308872, @koops wrote:

1. Taken care of some of the comments by Alexey.
2. Added extra tests of loop_bind_messages.cpp

However, generic_loop_messages.cpp & generic_loop_ast_print.cpp are present and provide a good coverage.

I rather doubt that these tests provide good coverage, since you're changing the directive kind here on the fly. This is a very new functionality, which was not tested before.
Add the tests for nesting of the regions, i.e. loop bind directive, enclosed in different regions, and diagnostics.

clang/lib/Sema/SemaOpenMP.cpp

340

What Ma1D stands for, what is the meaning of this abbrev? Add a comment.

clang/test/OpenMP/loop_bind_codegen.cpp

3

Add tests with save/load precompiled code.

clang/test/OpenMP/loop_bind_messages.cpp

4

Add tests with save/load precompiled code.

[koops]

However, generic_loop_messages.cpp & generic_loop_ast_print.cpp are present and provide a good coverage.

I rather doubt that these tests provide good coverage, since you're changing the directive kind here on the fly. This is a very new functionality, which was not tested before.
Add the tests for nesting of the regions, i.e. loop bind directive, enclosed in different regions, and diagnostics.

In https://reviews.llvm.org/D145823 : Add codegen for combined 'loop' directives, I see the following code
void CodeGenFunction::EmitOMPParallelGenericLoopDirective() {
....
....

emitCommonOMPParallelDirective(*this, S, OMPD_for, CodeGen, emitEmptyBoundParameters);

....
....
}
Here, the GenericLoopDirective is being changed to OMPD_for because of the Parallel Directive. Similarly,

void CodeGenFunction::EmitOMPTeamsGenericLoopDirective() {
....
....

CGF.CGM.getOpenMPRuntime().emitInlinedDirective(CGF, OMPD_distribute, CodeGenDistribute);

....
....
}
Here, GenericLoopDirective is being mapped to OMPD_distribute because of the Teams construct. These changes are very similar to mine. I still do not understand why you are saying "This is a very new functionality". If you can at least provide me some directions on which are all the "enclosed in different regions" it will be helpful.

[koops]

1. Addition of extra test case loop_bind_enclosed.cpp.
2. Changing of name from Map1D to MappedDirective

[koops]

This comment was removed by koops.

[ABataev]

clang/include/clang/Sema/Sema.h

11083

checkLastPrivateForMappedDirectives

clang/lib/AST/StmtPrinter.cpp

745–746

Just MappedDirective == llvm::omp::OMPD_loop

746–748

Remove braces

765–766

Just MappedDirective == llvm::omp::OMPD_loop

766–768

Remove braces

1005–1006

Just MappedDirective == llvm::omp::OMPD_loop

1006–1008

Remove braces

clang/lib/Sema/SemaOpenMP.cpp

343

I assume it shall be a member of SharingMapType, otherwise there might be problems with handling of enclosed constructs.

646

Why need a cast here?

647–648

use nullptr or just assert(Top &&

6140–6201

Could you outline this new code as a separate function?

10419

tmp is bad name. Give better name and follow LLVM coding style recommendations.

10461

Same

10474

Use c++ style of comments

10475–10476

if (DSAStack->getMappedDirective() == OMPD_loop && checkGenericLoopLastprivate(*this, Clauses, OMPD_loop, DSAStack)). Avoid structured complexity.

13993

Same, c++

13994–13995

Same, avoid complexity

13994–13998

`return DSAStack->getMappedDirective() == OMPD_loop && checkGenericLoopLastprivate(*this, Clauses, OMPD_loop, DSAStack);