Page MenuHomePhabricator

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

Authored by koops on Feb 23 2023, 3:14 AM.

Details

Summary

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

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

Diff Detail

Event Timeline

koops created this revision.Feb 23 2023, 3:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 23 2023, 3:14 AM
koops requested review of this revision.Feb 23 2023, 3:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 23 2023, 3:14 AM
koops added a comment.Feb 23 2023, 3:15 AM

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

We need tests.

clang/lib/CodeGen/CGStmtOpenMP.cpp
7790 ↗(On Diff #499780)

style, also below.

ronlieb added a subscriber: ronlieb.Mar 1 2023, 1:32 PM

LIT Test ?

ddpagan added a subscriber: ddpagan.Mar 1 2023, 3:28 PM

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

carlo.bertolli added inline comments.
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 added inline comments.Mar 6 2023, 3:10 AM
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 added inline comments.Mar 6 2023, 6:50 AM
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 added inline comments.Mar 6 2023, 10:13 AM
clang/lib/CodeGen/CGStmtOpenMP.cpp
7791 ↗(On Diff #499780)

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

koops updated this revision to Diff 505869.Mar 16 2023, 10:32 AM
  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 added inline comments.Mar 16 2023, 10:38 AM
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 updated this revision to Diff 512381.Apr 11 2023, 3:09 AM

Moving the code to SemaOpenMP.cpp from CodeGen.

koops updated this revision to Diff 512448.Apr 11 2023, 7:29 AM

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 added inline comments.Apr 11 2023, 7:35 AM
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 added inline comments.Apr 12 2023, 10:07 AM
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 added inline comments.Apr 12 2023, 10:53 AM
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 updated this revision to Diff 514096.EditedApr 16 2023, 10:35 PM
  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
  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.

Also, add AST printing tests

koops updated this revision to Diff 518341.Apr 30 2023, 11:37 AM
  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.

  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 added a comment.EditedTue, May 16, 9:56 AM

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 updated this revision to Diff 527127.EditedWed, May 31, 11:05 AM
  1. Addition of extra test case loop_bind_enclosed.cpp.
  2. Changing of name from Map1D to MappedDirective
koops added a comment.Wed, May 31, 9:41 PM
This comment was removed by koops.
ABataev added inline comments.Thu, Jun 1, 5:58 AM
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);