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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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? |
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. |
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.
mlir/lib/Conversion/OpenMPToLLVM/OpenMPToLLVM.cpp | ||
---|---|---|
145 | Please add tests for this in the following file. |
- 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
Please add tests for this in the following file.
mlir/test/Conversion/OpenMPToLLVM/convert-to-llvmir.mlir