[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#4263033 <https://reviews.llvm.org/D147172#4263033>, @jsjodin wrote:

> In D147172#4262744 <https://reviews.llvm.org/D147172#4262744>, @kiranchandramohan wrote:
>
>>>>>>> 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.
>>
>> I was making a general point that, we already have handling in the OpenMPIRBuilder that is capable of conditionally calling a parallel or sequential version. I was hoping that this can be used to generate the if-then-else for the offload.
>>
>> In general, the concern about not using the OutlineInfo mechanism is whether this will fail to compose with existing. code that uses the OutlineInfo Mechanism.
>>
>> But if you and @jdoerfert feel this is the right way to go then I don't have any issues.
>
> I think it can be done.
>
> In D147172#4262744 <https://reviews.llvm.org/D147172#4262744>, @kiranchandramohan wrote:
>
>>>>>>> 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.
>>
>> I was making a general point that, we already have handling in the OpenMPIRBuilder that is capable of conditionally calling a parallel or sequential version. I was hoping that this can be used to generate the if-then-else for the offload.
>>
>> In general, the concern about not using the OutlineInfo mechanism is whether this will fail to compose with existing. code that uses the OutlineInfo Mechanism.
>>
>> But if you and @jdoerfert feel this is the right way to go then I don't have any issues.
>
> I am unsure about how target regions can compose with the other directives that use the OutlineInfo. It might be possible to use the OutlineInfo mechanism, but I don't think it will work with some of the other existing utilities in the OpenMPIRBuilder e.g. emitTargetRegionFunction, which is used in this patch.

I think that reusing the offloading utilities is the better option right now. It will allow us to not re-implement a bunch of logic to make faster progress. Also if anything changes with respect to the omp runtime we wouldn't have to modify the code in two places. Since everything will be in the OpenMPIRBuilder we could do some refactoring later of if it seems worth doing.


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

https://reviews.llvm.org/D147172