This is an archive of the discontinued LLVM Phabricator instance.

[MLIR][OpenMP] Add support for depend clause
ClosedPublic

Authored by psoni2628 on Jan 27 2023, 8:17 AM.

Details

Summary

This patch adds support for the OpenMP 4.0 depend clause (in, out,
inout) of the task construct to the definition of the OpenMP MLIR
dialect and translation from MLIR to LLVM IR using OMPIRBuilder.

Diff Detail

Event Timeline

psoni2628 created this revision.Jan 27 2023, 8:17 AM
psoni2628 requested review of this revision.Jan 27 2023, 8:17 AM
psoni2628 set the repository for this revision to rG LLVM Github Monorepo.Jan 27 2023, 8:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 27 2023, 8:17 AM
kiranchandramohan requested changes to this revision.Jan 30 2023, 11:00 AM

Looks mostly good. A few questions/comments.

Would we be able to construct task graphs based on task dependencies? Or would this be possible only at runtime?
https://discourse.llvm.org/t/modeling-task-graphs-in-mlir/4080

Will this cover all kinds of dependencies?

Also call out in the summary that you are adding support for the depend clause in this patch.

mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
701

Nit: You can move this close to its use.

708

Nit: Can you add a comment why both map to DepInout?

725–726

Nit: What are these nullptrs?

mlir/test/Target/LLVMIR/openmp-llvm.mlir
2265–2269

Can we reduce the code here to the minimum?

This revision now requires changes to proceed.Jan 30 2023, 11:00 AM
psoni2628 updated this revision to Diff 494096.Feb 1 2023, 3:50 PM
  • Address review comments

Looks mostly good. A few questions/comments.

Would we be able to construct task graphs based on task dependencies? Or would this be possible only at runtime?
https://discourse.llvm.org/t/modeling-task-graphs-in-mlir/4080

Will this cover all kinds of dependencies?

Also call out in the summary that you are adding support for the depend clause in this patch.

I think you would be able to construct a task graph based on dependencies. In some situations where values are only known at run time, it may be challenging to figure out dependencies statically.

This covers all kinds of dependencies in OpenMP 4.0. There are newer ones like mutexinoutset introduced in OpenMP 5.0 that we can add in a later patch.

Could you please clarify your last point here? I believe the summary already mentions that the patch is adding support for the depend clause.

mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
708

Sure, I have added a comment. I wanted to add this commit as a reference for the comment: https://reviews.llvm.org/rG92e82f9cce62ae50e72530563811f0d3bd98e892.

mlir/test/Target/LLVMIR/openmp-llvm.mlir
2265–2269

I have removed the 2 unused arguments. Besides that, we could remove some of the non-depend-specific task code (e.g. outlined_fn) copied from the test above this one, but I think it is worth keeping to ensure the there are no regressions in that when using the depend clause. Let me know if you agree.

kiranchandramohan accepted this revision.Feb 3 2023, 6:18 AM

LG.

This covers all kinds of dependencies in OpenMP 4.0. There are newer ones like mutexinoutset introduced in OpenMP 5.0 that we can add in a later patch.

Could you clarify what is covered and what is not covered in the summary?

Could you please clarify your last point here? I believe the summary already mentions that the patch is adding support for the depend clause.

The summary only talks about translation.

This revision is now accepted and ready to land.Feb 3 2023, 6:18 AM
mlir/lib/Conversion/OpenMPToLLVM/OpenMPToLLVM.cpp
145

Please add tests for this in the following file.
mlir/test/Conversion/OpenMPToLLVM/convert-to-llvmir.mlir

psoni2628 edited the summary of this revision. (Show Details)Feb 6 2023, 12:23 PM
psoni2628 updated this revision to Diff 495256.Feb 6 2023, 12:49 PM
  • Fix a comment Print Reduction Clause -> Print Depend Clause
  • Add a test to mlir/test/Conversion/OpenMPToLLVM/convert-to-llvmir.mlir as suggested by reviewer comments

LG.

This covers all kinds of dependencies in OpenMP 4.0. There are newer ones like mutexinoutset introduced in OpenMP 5.0 that we can add in a later patch.

Could you clarify what is covered and what is not covered in the summary?

Could you please clarify your last point here? I believe the summary already mentions that the patch is adding support for the depend clause.

The summary only talks about translation.

Please let me know if the summary is good now.

This revision was automatically updated to reflect the committed changes.