This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP][IRBuilder] Add support for taskgroup
ClosedPublic

Authored by shraiysh on Jun 20 2022, 7:09 AM.

Details

Diff Detail

Event Timeline

shraiysh created this revision.Jun 20 2022, 7:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 20 2022, 7:09 AM
shraiysh updated this revision to Diff 438625.Jun 21 2022, 3:18 AM

Added tests.

Herald added a project: Restricted Project. · View Herald TranscriptJun 21 2022, 3:21 AM

One general question: can this be used for clang? Usually the clang side is also supported when supporting some new constructs/clauses in OMPIRBuilder?

shraiysh added a comment.EditedJun 27 2022, 7:07 AM

One general question: can this be used for clang? Usually the clang side is also supported when supporting some new constructs/clauses in OMPIRBuilder?

I think it can be used for clang. No clause is handled here and so I cannot replace clang's taskgroup generation with this function right now, but there is no special Fortran handling here.

One general question: can this be used for clang? Usually the clang side is also supported when supporting some new constructs/clauses in OMPIRBuilder?

I think it can be used for clang. No clause is handled here and so I cannot replace clang's taskgroup generation with this function right now, but there is no special Fortran handling here.

No need to replace clang. Here is one example(D114379). You can use -fopenmp-enable-irbuilder. It is good to have the initial running test using c/c++ code for OMPIRBuilder implementation. But this is not one hard suggestion considering our higher priority is to implement this for Flang.

One general question: can this be used for clang? Usually the clang side is also supported when supporting some new constructs/clauses in OMPIRBuilder?

I think it can be used for clang. No clause is handled here and so I cannot replace clang's taskgroup generation with this function right now, but there is no special Fortran handling here.

No need to replace clang. Here is one example(D114379). You can use -fopenmp-enable-irbuilder. It is good to have the initial running test using c/c++ code for OMPIRBuilder implementation. But this is not one hard suggestion considering our higher priority is to implement this for Flang.

Thanks for this. I will add clang code generation too.

Meinersbur accepted this revision.Jun 30 2022, 1:28 PM

Using it in Clang as well would certainly be nice in agreement with @peixin, I don't think required to get this patch in, or for the Flang implementation to wait until there is a use in Clang.

So LGTM. You could wait with committing until you have the Clang patch that verifies that it actually works end-to-end, but it is up to you.

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
633

[nit] Or using Fortran syntax. Or more generally "taskgroup construct"

llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
5000

While I dislike making the tests fragile to depend on the instruction order in the BB, I think this limited use is still acceptable.

5014

So glad you are using this function instead of walking the branch instructions! 👍

This revision is now accepted and ready to land.Jun 30 2022, 1:28 PM
shraiysh updated this revision to Diff 445437.Jul 18 2022, 2:45 AM

Rebase with main

This revision was automatically updated to reflect the committed changes.

The OpenMPIRBuilderTest.cpp test fails to build on AIX. https://lab.llvm.org/buildbot/#/builders/214/builds/2398/steps/6/logs/stdio Can you take a look please?

The OpenMPIRBuilderTest.cpp test fails to build on AIX. https://lab.llvm.org/buildbot/#/builders/214/builds/2398/steps/6/logs/stdio Can you take a look please?

Sure, I will take a look. Till then, please feel free to revert this change if required.

llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
5014

Thanks :)

The OpenMPIRBuilderTest.cpp test fails to build on AIX. https://lab.llvm.org/buildbot/#/builders/214/builds/2398/steps/6/logs/stdio Can you take a look please?

Also, I do not have a PowerPC machine. How do I reproduce this locally? Would just enabling the target in CMake do the trick?