Page MenuHomePhabricator

[OMPIRBuilder] Support depend clause for task construct
ClosedPublic

Authored by psoni2628 on Oct 11 2022, 10:04 AM.

Details

Summary

This patch adds OMPIRBuilder support for the depend clause for the
task construct.

Diff Detail

Event Timeline

psoni2628 created this revision.Oct 11 2022, 10:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 11 2022, 10:04 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
psoni2628 requested review of this revision.Oct 11 2022, 10:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 11 2022, 10:04 AM
tschuett added inline comments.
llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
210

The enums above are enum class.

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

Why is this not enum class?

psoni2628 added inline comments.Oct 11 2022, 10:11 AM
llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
649

There is some code duplication going on here. For example, this enum is basically copied from clang/include/clang/Basic/OpenMPKinds.def. I tried extracting this enum into llvm/include/llvm/Frontend/OpenMP/OMPConstants.h so that Clang can use it, but it seems that the OPENMP_DEPEND_KIND macro is complex and does not lend itself to a simple extraction. The macro is redefined in many different files. Any suggestions here would be appreciated.

psoni2628 updated this revision to Diff 466897.Oct 11 2022, 1:13 PM
  • Use enum class instead of enum for OpenMPDependKind and OMPRTLDependenceKindTy
psoni2628 updated this revision to Diff 467154.Oct 12 2022, 8:07 AM
  • Rebase, run clang-format
domada added inline comments.Oct 13 2022, 3:34 AM
llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
210

Do we need definition of OMPRTLDependenceKindTy and OpenMPDependKind ? It looks like these two class enums are overlapping.

psoni2628 added inline comments.Oct 13 2022, 8:16 AM
llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
210

OMPRTLDependenceKindTy is to be used with the OMPRTL___kmpc_omp_task_with_deps runtime function to encode what kind of dependency a given clause is, whereas OpenMPDependKind is for the MLIR to encode what kind of dependency a given clause is. You could map them to the same thing, but IMO, it is better to keep them separate. llvm::OpenMPIRBuilder::OpenMPDependKind::OMP_DEPEND_out and llvm::OpenMPIRBuilder::OpenMPDependKind::OMP_DEPEND_inout happen to map to the same thing, OMPRTLDependenceKindTy::OMPDepInOut, which is what the translateDependencyKind function does, but I don't think it makes much sense to map them to the same thing at the MLIR level. IMO, they should be kept distinct.

jdoerfert added inline comments.Oct 13 2022, 3:20 PM
llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
210

I doubt we want duplication. It doesn't buy as anything and introduces confusion.

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

I don't understand the problem. Define all depend kinds similar to proc bind here https://github.com/llvm/llvm-project/blob/25162418c60475185cf29b5336015ca2f52ce3eb/llvm/include/llvm/Frontend/OpenMP/OMPKinds.def#L1052 . Then use that to create the enum class in OMPConstants.h and use that enum class as well as the macro everywhere, including clang.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
1521

Is the insert point set to the right place for sure?

1556

Generally, llvm:: inside llvm/ if we open the namespace anyway.

psoni2628 updated this revision to Diff 467942.Oct 14 2022, 3:29 PM
  • Remove unnecessary llvm:: from OMPIRBuilder.cpp
  • Change the insertion point for DepArray alloca to be before the Then basic block for the if clause
  • Use only one enum RTLDependenceKindInfoTy to encode the kind of dependency; use this enum in Clang
llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
649

Thanks for the suggestion! However, if we don't want to have 2 enums, one for the clause and one for the RTL function, as you mentioned in one of your other comments (quoted below), then I can just define a singular enum in OMPConstants.h. IMO, this is simpler.

I doubt we want duplication. It doesn't buy as anything and introduces confusion.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
1521

Looking more closely at it, I think this should be inserted before the Then of the IfThenElse for the if clause, but then the OMPRTL___kmpc_omp_task_with_deps call should still be in the Then.

Generally fine, I think. The alloca placement is the last bit that needs changing.

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

If these are all uses, I assume a fixed enum is good enough.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
1441

Same question as before. How do you ensure the alloca is placed where you want it (which is the function entry block).

1521

The entire conditional here is a bad idea. We should pass the if-clause value to the runtime. This causes all sort of problems, but it's unrelated to this patch...

psoni2628 added inline comments.Oct 14 2022, 9:02 PM
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
1441

Isn't the insertion point set on L1356 in OMPIRBuilder.cpp at the caller of the outlined function, i.e. the outlined function entry block? I thought it made sense to do the alloca in this same block, right after the __kmpc_omp_task_alloc. If not this one, which function entry block are you suggesting I should place the alloca in?

jdoerfert added inline comments.Oct 17 2022, 9:13 AM
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
1441

Reading L1356, it looks like it is set to the call site of outlined function, which could be anywhere in current function, no? Allocas that only need to provide a single memory location for each function invocation should be placed in the function entry block.

psoni2628 updated this revision to Diff 468560.Oct 18 2022, 8:28 AM
  • Set insertion point for DependInfo array alloca to the function entry block, then restore the original insertion point
psoni2628 added inline comments.Oct 18 2022, 8:33 AM
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
1441

That makes sense. Thanks for the clarification.

jdoerfert accepted this revision.Oct 18 2022, 10:15 AM

LG, one nit.

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

Add doxygen documentation for this class please.

This revision is now accepted and ready to land.Oct 18 2022, 10:15 AM
psoni2628 updated this revision to Diff 468708.Oct 18 2022, 2:50 PM
  • Add doxygen comment for DependData struct