[PATCH] D147172: [OpenMP][Flang][MLIR] Add lowering of TargetOp for host codegen

[Jan Sjödin via Phabricator via llvm-commits]

jsjodin added a comment.

In D147172#4246212 <https://reviews.llvm.org/D147172#4246212>, @kiranchandramohan wrote:

> In D147172#4245879 <https://reviews.llvm.org/D147172#4245879>, @jsjodin wrote:
>
>> In D147172#4244736 <https://reviews.llvm.org/D147172#4244736>, @jsjodin wrote:
>>
>>> In D147172#4240342 <https://reviews.llvm.org/D147172#4240342>, @kiranchandramohan wrote:
>>>
>>>> In D147172#4237283 <https://reviews.llvm.org/D147172#4237283>, @jsjodin wrote:
>>>>
>>>>> @kiranchandramohan I moved the outlining to the OpenMPIRBuilder, is this more in line of what you were thinking? There is a patch up by @TIFitis for Fortran->Fir lowering https://reviews.llvm.org/D147339. I will prune this patch to only have the OMP+LLVM Dialect -> LLVMIR portions and  appropriate tests.
>>>>
>>>> Thanks @jsjodin for the changes. Yes, moving the code to the OpenMPIRBuilder looks fine. I have not looked at the patch in detail. But I see that you have not used the OpenMPIRBuilder's outlining facility (OutlineInfo(s) and the PostOutlineCB). Are there any issues in using these here?
>>>
>>> I have reworked the patch a bit to simplify things and added tests. The reason why the OutlineInfos etc are not used for the target regions is probably because the outlining is more complicated. With offloading we have to generate an if-then-else that tries to offload via the openmp runtime and if the runtime fails to launch the kernel the fallback is executed instead.
>>
>> Also since the outlining is shared with the device codegen, that is also a reason why the OutlineInfos etc is not used.
>
> We have handling for the `if clause` in both task and parallel. Will the handling in Target be substantially different from these?
> Task : D130615 <https://reviews.llvm.org/D130615>
> Parallel : D138495 <https://reviews.llvm.org/D138495> (The latest revision uses a runtime extension, so that we do not have to generate IR for both sequential and parallel version).

The if-then-else is not related to the if clause, it is only for the host fallback. The code in this patch will be updated once the kernel argument handling is implemented for the offloading case. We will have to add the if clause handling as well later on.



================
Comment at: mlir/test/Target/LLVMIR/omptarget-llvm.mlir:178
+
+module attributes {dlti.dl_spec = #dlti.dl_spec<#dlti.dl_entry<"dlti.endianness", "little">, #dlti.dl_entry<i64, dense<64> : vector<2xi32>>, #dlti.dl_entry<f80, dense<128> : vector<2xi32>>, #dlti.dl_entry<i1, dense<8> : vector<2xi32>>, #dlti.dl_entry<i8, dense<8> : vector<2xi32>>, #dlti.dl_entry<i16, dense<16> : vector<2xi32>>, #dlti.dl_entry<i32, dense<32> : vector<2xi32>>, #dlti.dl_entry<f16, dense<16> : vector<2xi32>>, #dlti.dl_entry<f64, dense<64> : vector<2xi32>>, #dlti.dl_entry<f128, dense<128> : vector<2xi32>>>, fir.defaultkind = "a1c4d8i4l4r4", fir.kindmap = "", llvm.data_layout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128", llvm.target_triple = "x86_64-unknown-linux-gnu", omp.is_device = #omp.isdevice<is_device = false>} {
+  llvm.func @omp_target_region_() {
----------------
TIFitis wrote:
> I think the module can be omitted from the test.
It has to stay because of omp.is_device = #omp.isdevice<is_device = false>


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147172/new/

https://reviews.llvm.org/D147172