[PATCH] D94871: [Clang][OpenMP] Fixed an issue that clang crashed when compiling OpenMP program in device only mode without host IR

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:2944-2947
+    // This could happen if the device compilation is invoked standalone.
+    if (!hasTargetRegionEntryInfo(DeviceID, FileID, ParentName, LineNum))
+      initializeTargetRegionEntryInfo(DeviceID, FileID, ParentName, LineNum,
+                                      OffloadingEntriesNum);
----------------
tianshilei1992 wrote:
> ABataev wrote:
> > jdoerfert wrote:
> > > ABataev wrote:
> > > > tianshilei1992 wrote:
> > > > > ABataev wrote:
> > > > > > jdoerfert wrote:
> > > > > > > tianshilei1992 wrote:
> > > > > > > > ABataev wrote:
> > > > > > > > > jdoerfert wrote:
> > > > > > > > > > ABataev wrote:
> > > > > > > > > > > I would add a chack that to auxiliary device was specified. And if it was specified, it means this is not device-only mode and still need to emit an error.
> > > > > > > > > > No it doesn't. There is nothing wrong with https://godbolt.org/z/T1h9b5, and as I said before, I can build the situation in various other ways as well, some of which will be outside of the users control. A global can exist in the host/device code only.
> > > > > > > > > I'm not saying that this is wrong. This code was used to check that the compiler works correctly and it just allows developer to understand that there is a problem with the compiler if it misses something and there is a difference between host and device codegens. If we don't want to emit an error here, still would be good to have something like an assert to be sure that the host/device codegens are synced.
> > > > > > > > That check still doesn't work for the test case provided by @jdoerfert because host IR doesn't contain that global in the offload info.
> > > > > > > As @tianshilei1992 says, my test case does show how this can never be an assertion/warning even for regular host+device compliation. There is no guarantee a host version exists, or a device one does. We need to gracefully allow either.
> > > > > > So, this is caused by the `nohost` variant selector, right? In this case we don't emit it for the host and don't have corresponding entry in the metadata?
> > > > > Correct.
> > > > Ok, now I see. Is it possible to distinguish this corner case from the general one, where the variable is available for both, host and device, or it will require some extra work/design? If it is easy to differentiate these 2 cases, it would be good to keep something like an error message/assert for the general case and remove the restriction for this corner case. Otherwise, proceed with the current solution.
> > > FWIW, I can also use ifdef, or various other context selectors to achieve the same effect.
> > Ok, then go ahead with this solution.
> I'm thinking we can probably set a new option to invoke device only compilation, like `--cuda-device-only` we already had for CUDA. However, the thing is, unlike CUDA, we cannot tell which parts are kernels implicitly w/o host compilation. So if we provide the option, we might end up with only recognizing code in `declare target` or all its variant.
I think the host part is not really required here. IIRC, it is used currently only to check that the host code and the device code are synced and device part matches the host codegen. If we need device-only codegen, we can just turn off this check. At least, you can try.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94871