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

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan added a comment.



>>>>> 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.



================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4180
+  // TODO: Add kernel launch call when device codegen is supported.
+  Builder.CreateCall(OutlinedFn, Args);
+}
----------------
jsjodin wrote:
> jsjodin wrote:
> > jdoerfert wrote:
> > > FWIW, you should already (be able to) emit the kernel launch call and fallback handling.
> > > The latter should kick in as the symbol is neither registered nor available on the device.
> > > FWIW, you should already (be able to) emit the kernel launch call and fallback handling.
> > > The latter should kick in as the symbol is neither registered nor available on the device.
> > 
> > Should I add the code to build the kernel args, or do you mean empty kernel args?
> I forgot to mention that I am working on a patch to handle the kernel args which adds the kernel launch call. So I could pull that merge that patch into this or keep it separate. @kiranchandramohan, @jdoerfert  you have a preference?
No. I don't have a preference.


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

https://reviews.llvm.org/D147172