This is an archive of the discontinued LLVM Phabricator instance.

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

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
6124

Remove this new line

6181–6189
  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
1

Remove this

51–56

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
51–56

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
9792

err_omp_...

clang/lib/Sema/SemaOpenMP.cpp
6146

ParentDirective

6167–6168

Remove extra parens

6173–6175

Remove extra braces

6181–6189

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.

6199–6202

Why need to drop bind clause here?

clang/test/OpenMP/loop_bind_codegen.cpp
51–56

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
638

NewDK

640

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
1–2 ↗(On Diff #514096)
  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
339

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
3 ↗(On Diff #518341)

Add tests with save/load precompiled code.

koops added a comment.EditedMay 16 2023, 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.May 31 2023, 11:05 AM
koops added a comment.EditedMay 31 2023, 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.May 31 2023, 9:41 PM
This comment was removed by koops.
ABataev added inline comments.Jun 1 2023, 5:58 AM
clang/include/clang/Sema/Sema.h
11083 ↗(On Diff #527127)

checkLastPrivateForMappedDirectives

clang/lib/AST/StmtPrinter.cpp
745–746 ↗(On Diff #527127)

Just MappedDirective == llvm::omp::OMPD_loop

746–748 ↗(On Diff #527127)

Remove braces

765–766 ↗(On Diff #527127)

Just MappedDirective == llvm::omp::OMPD_loop

766–768 ↗(On Diff #527127)

Remove braces

1005–1006 ↗(On Diff #527127)

Just MappedDirective == llvm::omp::OMPD_loop

1006–1008 ↗(On Diff #527127)

Remove braces

clang/lib/Sema/SemaOpenMP.cpp
342

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

640

Why need a cast here?

641–642

use nullptr or just assert(Top &&

6136–6197

Could you outline this new code as a separate function?

10403

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

10438

Same

10449

Use c++ style of comments

10450–10451

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

13962

Same, c++

13963–13964

Same, avoid complexity

13963–13967

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

koops updated this revision to Diff 529532.EditedJun 8 2023, 1:44 AM
  1. Taken care of Alexy's comments.
  2. Reverting back changes to StmtPrinter.cpp because state of AST needs to be shown as is to the developer of clang when -ast-print/-ast-dump is used. This also meant some changes to the test cases to reflect the Mapped directives (from "omp loop" to "omp for", "omp distribute" or "omp simd").
ddpagan added inline comments.Jun 22 2023, 8:30 PM
clang/include/clang/Basic/DiagnosticSemaKinds.td
9793

Should be "allowed".

clang/lib/Sema/SemaOpenMP.cpp
6121

This function only applies to OMPD_loop. If *Kind != OMPD_loop then should just return UseClauseWithoutBind.

clang/test/OpenMP/generic_loop_ast_print.cpp
26 ↗(On Diff #529532)

The AST that is printed should be what was originally specified in the source.

clang/test/OpenMP/loop_bind_codegen.cpp
2

Should auto-generate the the IR checking for this test using llvm/utils/update_cc_test_checks.py after verifying that what's generated is correct.

ABataev added inline comments.Jun 27 2023, 5:40 AM
clang/include/clang/Sema/Sema.h
11094 ↗(On Diff #529532)

const member function?
Add a comment

11095 ↗(On Diff #529532)

Add a comment

11095 ↗(On Diff #529532)

SmallVectorImpl<OMPClause *> & instead of pointer to fully specified SmallVector

koops added inline comments.Jul 5 2023, 9:25 PM
clang/test/OpenMP/generic_loop_ast_print.cpp
26 ↗(On Diff #529532)

Since I am mapping the loop directive to new directives (in the Sema itself) I am trying to print the state of AST as is to the developer of clang when -ast-print/-ast-dump is used. Without this correct printing of the state of the AST (new directives)) the developer may wonder why the code is not behaving as he/she specified and also in which phase the code gets changed.

koops added inline comments.Jul 5 2023, 11:41 PM
clang/include/clang/Sema/Sema.h
11094 ↗(On Diff #529532)

"checkLastPrivateForMappedDirectives" is calling a non-const function "checkGenericLoopLastprivate". Since, this is a pre-existing function and is being called from other locations, I cannot make "checkLastPrivateForMappedDirectives" a const.

koops updated this revision to Diff 539017.Jul 11 2023, 4:20 AM
  1. Taking care of Alexy & David suggestions: a) Using update_cc_test_checks.py to generate CHECK statements. b) Change in error message from "handled" to "allowed". c) Adding comments for the bind clause. d) Mangled names of the functions in the CHECK statements are more generic with regular expressions.
ABataev added inline comments.Jul 11 2023, 6:18 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
9793

'reduction'

clang/lib/Sema/SemaOpenMP.cpp
6116

Why ClausesWithoutBind passing by pointer? Use llvm::SmallVectorImpl<OMPClause *> &ClausesWithoutBind instead

6118

Same, pass by reference

13982–13983

Restore formatting

koops updated this revision to Diff 540373.Jul 14 2023, 5:26 AM

Addressing Alexey's comments.
The name matching in loop_bind_messages.cpp & generic_loop_codegen.cpp tests changed to take care of the failures.

ABataev added inline comments.Jul 14 2023, 5:32 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
9790

Also, 'loop'

clang/include/clang/Sema/Sema.h
11136 ↗(On Diff #540373)

llvm::SmallVectorImpl<OMPClause *> &ClausesWithoutBind

koops updated this revision to Diff 541155.Jul 17 2023, 11:36 AM
  1. Removing the size for the template constructor, llvm::SmallVectorImpl<OMPClause *> &ClausesWithoutBind.
  2. In the diagnostic message of err_omp_bind_required_on_loop, using single quotes for the name 'loop' construct.
  3. Minor changes to the tests clang/test/OpenMP/loop_bind_codegen.cpp & clang/test/OpenMP/loop_bind_enclosed.cpp to use a generic pattern matching expression for the names & reducing some checks.
ABataev added inline comments.Jul 17 2023, 11:39 AM
clang/lib/Sema/SemaOpenMP.cpp
342

Not done

koops updated this revision to Diff 544428.Jul 26 2023, 10:40 AM

Moving the variable MappedDirective into structure SharingMapTy. Enough comments have been put in place to explain.
Modified some of the loop_bind tests to cater to windows and debian platform.

ABataev added inline comments.Jul 26 2023, 11:28 AM
clang/include/clang/AST/StmtOpenMP.h
291 ↗(On Diff #544428)

OpenMPDirectiveKind PrevMappedDirective = llvm::omp::OMPD_unknown;

308 ↗(On Diff #544428)

Remove initialization of prevMappedDirective with the default value from the constructor

616 ↗(On Diff #544428)

const member function

clang/include/clang/Sema/Sema.h
11476 ↗(On Diff #544428)

All these changed functions are called with the default value of this new prevMappedDirective paramer. Remove.

clang/lib/Sema/SemaOpenMP.cpp
6365

Can you just use DSAStack->getMappedDirective() instead of prevMappedDirective?

10438

auto *ForDirective

koops updated this revision to Diff 544542.Jul 26 2023, 3:35 PM

Fixing Alexey's suggestions.

koops updated this revision to Diff 544688.Jul 27 2023, 3:32 AM

Correction for clang-format errors.

ABataev added inline comments.Jul 27 2023, 4:15 AM
clang/lib/Sema/TreeTransform.h
1652 ↗(On Diff #544688)

PrevMappedDirective

koops updated this revision to Diff 544892.Jul 27 2023, 12:38 PM
  1. Converting prevMapped to PrevMappedDirective
  2. Reducing the CHECK patterns in loop_bind_enclosed.cpp to the most essential ones to succeed matching for the IR generation on windows.
koops updated this revision to Diff 545728.EditedJul 31 2023, 10:17 AM
  1. clang-format error in clang/include/clang/AST/StmtOpenMP.h corrected.
  2. Correcting clang/test/OpenMP/loop_bind_enclosed.cpp to use regular expressions for names of functions to accomodate names generated on Microsoft Windows.
ABataev added inline comments.Jul 31 2023, 12:24 PM
clang/include/clang/AST/StmtOpenMP.h
291 ↗(On Diff #545728)

I don't see where this field is stored/loaded for PCH support. You need add a support for this in Serialization.

611–614 ↗(On Diff #545728)

Better to make it a part of Create member function rather than having a separate function for this.

koops updated this revision to Diff 546159.Aug 1 2023, 11:32 AM
  1. clang/test/OpenMP/loop_bind_enclosed.cpp : Converting main and function names within it to use regular expressions to accomodate windows platform.

i) Instead of calling setMappedDirective() after the the creation of the Directive, made the MappedDirective a parameter of the Create() method.
ii) setMappedDirective() is called from the Directive::Create()
iii) The inheritance is : OMPExecutableDirective --> OMPLoopBasedDirective --> OMPLoopDirective --> OMPSimdDirective, from OMPSimdDirective constructor it involves a lot of changes to initialize PrevMappedDirective variable in OMPExecutableDirective starting from OMPSimdDirective, even if the variable is "protected". Hence we call setMappedDirective() from OMPSimdDirective::Create().

koops added inline comments.Aug 1 2023, 11:39 AM
clang/include/clang/AST/StmtOpenMP.h
291 ↗(On Diff #545728)

Since the ASTReader and ASTWriter are used by the developer of clang, it is preferable to show the internal state of the compiler as is, e.g. if "#pragma omp loop bind(parallel)" is changed to "#pragma omp for" then the ASTWriter has to show it as "#pragma omp for". I can change it to "#pragma omp loop bind(parallel)" using the MappedDirective that is stored however, that would be misleading to the developer of clang.

ABataev added inline comments.Aug 1 2023, 11:45 AM
clang/include/clang/AST/StmtOpenMP.h
291 ↗(On Diff #545728)

How it is related to PCH? Try to serialize/deserialize the template and then try to create a new instance. There will be an issue since there is no serialization support, when you try to create new instance in the Rebuild... function (mapLoopConstruct will use OMPD_unknown as Prev value instead of oringal previous value, since it is not serialized/deserialized)

koops updated this revision to Diff 547304.Aug 4 2023, 12:10 PM
  1. Support for PCH Serilization/deserilization for the PrevMapLoopConstruct variable in OMPExecutableDirective.
  2. Modification of clang/test/PCH/pragma-loop.cpp to include "#pragma omp loop bind" with different parameters for bind clause.
This revision is now accepted and ready to land.Aug 4 2023, 12:13 PM
koops updated this revision to Diff 547905.Aug 7 2023, 12:21 PM

In clang/test/OpenMP/loop_bind_enclosed.cpp Generalizing the CHECK pattern for aarch64-linux, s390x-linux and ppc64le-linux.

koops updated this revision to Diff 548499.Aug 9 2023, 12:48 AM
koops added a reviewer: thakis.

Making the CHECK pattern generic to match platforms tested after committing changes.

koops added a comment.Aug 9 2023, 9:18 AM

Can someone please check for MacOS? Yesterday when this support was committed, the CHECK statements in tests loop_bind_codegen.cpp and loop_bind_enclosed.cpp had failed on Mac and I received a comment as follows:

When relanding, please remember to put a link to the review in the commit message.

I have made the CHECK statements generic enough to match those on Mac with CHECK for the functions that the test were expected.

Can someone please check for MacOS? Yesterday when this support was committed, the CHECK statements in tests loop_bind_codegen.cpp and loop_bind_enclosed.cpp had failed on Mac

I patched this in and verified that check-clang now passes on macOS. Thanks!

and I received a comment as follows:

When relanding, please remember to put a link to the review in the commit message.

To do this, run git commit --amend and put Differential Revision: https://reviews.llvm.org/D144634 at the bottom of the commit message. git log has many, many examples of this :)