This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP][5.1] Fix parallel masked is ignored #59939
ClosedPublic

Authored by randreshg on Feb 7 2023, 1:52 PM.

Details

Summary

Code generation support for 'parallel masked' directive.

The EmitOMPParallelMaskedDirective was implemented.
In addition, the appropiate device functions were added.

Fix #59939.

Diff Detail

Event Timeline

randreshg created this revision.Feb 7 2023, 1:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 7 2023, 1:52 PM
randreshg requested review of this revision.Feb 7 2023, 1:52 PM

Can you add a test please?

tianshilei1992 added inline comments.Feb 7 2023, 2:28 PM
clang/lib/CodeGen/CGStmt.cpp
431

also point out which directive is not supported.

I’m confused that Shilei said in the issue in GitHub that this is supported already. But front end is just throwing an error message.

Also. Should the same CGM.ErrorUnsupported be applied to all the llvm_unreachable in that switch

I’m confused that Shilei said in the issue in GitHub that this is supported already. But front end is just throwing an error message.

Our Sema can support it but CodeGen doesn't. Here we want to error it out instead of "silently" pass if assertion is disabled.

Also. Should the same CGM.ErrorUnsupported be applied to all the llvm_unreachable in that switch

That might be a good idea.

tianshilei1992 edited the summary of this revision. (Show Details)Feb 7 2023, 4:26 PM
tianshilei1992 added inline comments.
clang/lib/CodeGen/CGStmt.cpp
431

We might want a more detailed description, like the code gen of xxx is not supported.

Thanks for the clarification Shilei

randreshg added inline comments.Feb 7 2023, 6:29 PM
clang/lib/CodeGen/CGStmt.cpp
431

The CGM.ErrorUnsupported function already prints out that information. I can create another function that explicitly errors out the message you suggest. WDYT?

tianshilei1992 added inline comments.Feb 7 2023, 6:50 PM
clang/lib/CodeGen/CGStmt.cpp
431

Yeah, that's what I was referring to. It only says "this directive". I'd prefer explicit message.

This is not a fix. It's "progress" in the sense that we should not have unreachable in the first place but proper errors. That said, it still doesn't solve the issue and certainly doesn't fix https://github.com/llvm/llvm-project/issues/59939. See my comments and use https://godbolt.org/z/q7MaKbarv as test for this.

clang/lib/CodeGen/CGStmt.cpp
248

... here. And see what happens.

430

Move this line...

randreshg added inline comments.Feb 8 2023, 7:36 PM
clang/lib/CodeGen/CGStmt.cpp
248

The front-end crashes in the cast to OMPParallelMasterDirective. I guess the best solution would be to implement EmitOMPParallelMaskedDirective function. WDYT?

The parallel masked directive is emitted as a combined directive that consists of two implicit directives: 'parallel' with 'masked' directive.

jdoerfert added inline comments.Mar 23 2023, 2:27 PM
clang/test/OpenMP/parallel_masked.cpp
2

add the flag to include generated functions.

Just a suggestion, I think this works, although it may be best to use OpenMPIRBuilder.

clang/lib/CodeGen/CGStmtOpenMP.cpp
4492

I noticed that in EmitOMPParallelDirective and EmitOMPMaskedDirective there is an option to use OpenMPIRBuilder, instead of the traditional front end infrastructure.

Would it make sense to use this instead?

randreshg updated this revision to Diff 507910.Mar 23 2023, 3:54 PM

Generated functions included in the test

randreshg added inline comments.Mar 23 2023, 3:57 PM
clang/lib/CodeGen/CGStmtOpenMP.cpp
4492

what do you think @jdoerfert ?

jdoerfert added inline comments.Mar 23 2023, 4:22 PM
clang/lib/CodeGen/CGStmtOpenMP.cpp
4492

I think we need to move it at some point. Didn't we move some of these already?

clang/test/OpenMP/parallel_masked.cpp
36

Did you try to run this? I would guess it fails to link because this function is missing in the device runtime.

randreshg added inline comments.Mar 23 2023, 4:36 PM
clang/lib/CodeGen/CGStmtOpenMP.cpp
4492

I'll check and come back to you if any questions

clang/test/OpenMP/parallel_masked.cpp
36

Yes, I did, and it worked. When you say it would fail to link in the device runtime, would that be the case where the parallel masked region is inside a target region? If that is what you mean, I haven't tested that case yet.

jdoerfert added inline comments.Mar 23 2023, 5:25 PM
clang/test/OpenMP/parallel_masked.cpp
36

Yes, that is what I meant. Sorry. We can do that in a follow up or in this patch.

randreshg updated this revision to Diff 508277.Mar 24 2023, 8:53 PM

Adding masked support to the device

randreshg added inline comments.Mar 24 2023, 9:01 PM
clang/lib/CodeGen/CGStmtOpenMP.cpp
4492

I took a look at this and I think it would be better if we do it in a follow-up patch. The way I see it, in that patch, we can use OpenMPIRBuilder not only for EmitOMPParallelMasked but also for any other directive that uses the emitCommonOMPParallelDirective function, e.g, parallel for or parallel sections. WDYT? @josemonsalve2 @jdoerfert

josemonsalve2 added inline comments.Mar 24 2023, 9:36 PM
clang/lib/CodeGen/CGStmtOpenMP.cpp
4492

Fine by me. The patch looks good to me but let’s wait for the big boss to respond.

jdoerfert accepted this revision.Mar 28 2023, 3:09 PM

LG, minor nits, see below.

clang/test/OpenMP/parallel_masked.cpp
12

There is no test for the filter clause.

openmp/libomptarget/DeviceRTL/include/Interface.h
263

Style

openmp/libomptarget/DeviceRTL/src/Synchronization.cpp
523

In a follow up we should finally fix/cleanup the TId stuff. We still pass in the "level 0/1" TId and hope for the best. We could even use that if we knew we were in Lvl 0/1, that is, no nested parallel.
However, we generally don't. So we either generate better code to determine the TId we pass in, or have a special function here that uses TId for LVL 0/1 and the constant 0 otherwise. This could actually improve performance for a lot of the APIs as we had to replace the TId that is passed in all over the place to make it correct, but we don't always optimize the omp_get_thread_num out, I'd assume. Maybe we do and this is a non-issue, though we should check.

525

Style: Filter

This revision is now accepted and ready to land.Mar 28 2023, 3:09 PM
randreshg updated this revision to Diff 509893.Mar 30 2023, 8:52 PM

Test with filter clause added

randreshg updated this revision to Diff 510362.Apr 2 2023, 1:01 PM

Test fixed

randreshg updated this revision to Diff 510513.Apr 3 2023, 8:37 AM

Test updated.

randreshg updated this revision to Diff 510558.Apr 3 2023, 10:55 AM

Test updated.
This should work 🤞

josemonsalve2 edited the summary of this revision. (Show Details)Apr 3 2023, 1:25 PM
This revision was automatically updated to reflect the committed changes.
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 3 2023, 1:36 PM