Add basic translation of acc.data to LLVM IR with runtime calls.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
Apologies for the delay.
I had a quick look through the patch. I feel like a lot of the code here can be put into the OpenACC/OpenMP IRBuilder. As you know the usual way the function for a construct is written in the OpenMP IRBuilder is like there is a function (createData) for creating the runtime calls for the construct and there is a bodygen callback that can fill in the body of the construct. The createData function in the OpenACC IRBuilder can create the runtime calls for beginMapperFunc, endMapperFunc. Is it possible to rewrite the code like this?
mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp | ||
---|---|---|
275–315 | Can these two functions be in the OpenACC IRBuilder? | |
376–382 | This should be in the OpenACC IRBuilder I think. | |
386–422 | This code can probably be the bodygen callback, which creates the mapping from mlir to llvm blocks and then converts them to LLVM IR. | |
425–433 | This should be in the OpenACC IRBuilder I think. |
Thanks for the comments. At first I was thinking to put this in the OpenXXIRBuilder like other functions you mentioned. The only reason I did not is that even it would be in the IRBuilder, the clang code gen is using the CodeGenFunction to create the allocas through call to CreateMemTemp and this is likely not do-able in our case from MLIR. But if it is fine that the function the IRBuilder is only (partially at least) used from Flang then I 100% fine following your suggestion.
@kiranchandramohan I moved some of the code to the OpenMPIRBuilder. I left the region conversion in the translation for the moment. This region is a bit special since it could even be just inline between the two calls.
Thanks @clementval. I would request @jdoerfert, @Meinersbur, @fghanim to provide the feedback here.
MLIR part LGTM with comments addressed.
mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp | ||
---|---|---|
131–132 | We usually pass SmallVectorImpl & that avoids the need to fix the number of stack elements. SmallVector has a fixed number even if this number is computed by template deduction. | |
285 | No need for template here. | |
291–296 | Please expand auto here. MLIR uses auto when the type is obvious from the RHS, e.g., in a dyn_cast, or when it is too long or impossible to spell out, e.g. iterators and lambdas. | |
389–391 | It doesn't look like the insertion point changed between where currIP was created and here. |
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp | ||
---|---|---|
39 | If we have a struct with 3 alloca members with proper names this is not needed anymore. | |
2264 | Also elsewhere. | |
2281 | The types should all exist, Int8Ptr is a global in the omp namespace. If not, you can also add them to OMPKinds.def (top). I don't think returning a SmallVector is great. I'd suggest a struct with 3 Alloca members with names or pass such a struct in and fill it. Alloca IP has to be passed in from the caller. The entry point is not necessarily the right place, e.g., if this is called in a parallel region. |
clang-tidy: warning: invalid case style for variable 'kArgsBasePos' [readability-identifier-naming]
not useful