[PATCH] D158462: [OMPIRBuilder] Fix shared clause for task construct

[Prabhdeep Soni via Phabricator via llvm-commits]

psoni2628 added a comment.

In D158462#4631899 <https://reviews.llvm.org/D158462#4631899>, @kiranchandramohan wrote:

> In D158462#4614095 <https://reviews.llvm.org/D158462#4614095>, @psoni2628 wrote:
>
>> In D158462#4610644 <https://reviews.llvm.org/D158462#4610644>, @psoni2628 wrote:
>>
>>> In D158462#4610370 <https://reviews.llvm.org/D158462#4610370>, @jdoerfert wrote:
>>>
>>>> In D158462#4610257 <https://reviews.llvm.org/D158462#4610257>, @kiranchandramohan wrote:
>>>>
>>>>> In D158462#4610220 <https://reviews.llvm.org/D158462#4610220>, @psoni2628 wrote:
>>>>>
>>>>>>> As for the clang failures, it looks like they are related. I think I just need to update the CHECK lines, but I'm still figuring that out.
>>>>>>
>>>>>> The clang test failures are being caused by my addition of `kmp_task_t` to `OMPKinds.def`. It is conflicting with Clang's definition of `kmp_task_t` in `clang/lib/CodeGen/CGOpenMPRuntime.cpp`, so the struct gets renamed to `kmp_task_t.0` and `kmp_task_t.1`. I don't think it is that simple to update the usage of `kmp_task_t` in `clang/lib/CodeGen/CGOpenMPRuntime.cpp`. Should I rename my addition of `kmp_task_t` in `OMPKinds.def` to `kmp_task`, or should I just fix the tests to allow `kmp_task_t.0` and `kmp_task_t.1`?
>>>>
>>>> Can we make use of the new `kmp_task_t` in Clang? That would be the correct way to resolve this.
>>>
>>> I am not sure how to go about doing that. I think the `kmp_task_t` struct is actually being created in `llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:initializeTypes` from the macro defined in `OMPKinds.def`. Do you know how I can refer to that struct in Clang?
>>
>> I looked at this more closely, and I don't think it's possible to refer to `kmp_task_t` in Clang. The only way I can think of doing this is doing similarly to D123460 <https://reviews.llvm.org/D123460>, which is basically building the structure in OMPIRBuilder, and calling it in Clang. This is not a straightforward change to make though, because Clang builds several `RecordDecl`s on top of `kmp_task_t` such as the taskloop_task and task_with_privates. We would also have to build those structs in OMPIRBuilder, but that expands the scope of this patch quite significantly. Is this what you meant @jdoerfert?
>
> I think we should proceed with either of the following two options:
> -> Reuse Clang's `kmp_task_t` in the OpenMPIRBuilder and use only the relevant fields that we support now. I am assuming even if we have to move the definition of the Structure to OpenMPIRBuilder, there will not be much change in clang since the definition of the Structure remains the same. It is just a bit of additional logical to correctly dereference the portion of the structure in the OPenMPIRBuilder.
> -> Define a separate `kmp_task_ompbuilder_t` and use that in the OpenMPIRBuilder.

I don't think option 1 is possible. The struct gets created in https://github.com/llvm/llvm-project/blob/c987f9d7fdc7b22c9bf68d7b3f0df10b68c679be/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp#L5680. IMO, the only way this makes sense is to create a function in OMPIRBuilder to populate the `kmp_task_t`, and then call that in Clang (similar to D123460 <https://reviews.llvm.org/D123460>). That is straightforward, but the issue is that we currently only populate the `shareds` field of `kmp_task_t` in OMPIRBuilder. We need to populate the other fields as well, otherwise we will lose the functionality in Clang. Once we have the full support for all the features of `kmp_task_t` in OMPIRBuilder, then we can use it. Until that happens we can just use a different struct name (i.e. the second option).


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

https://reviews.llvm.org/D158462