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

[tianshilei1992]

Summary

D94745 rewrites the deviceRTLs using OpenMP and compiles it by directly
calling the device compilation. clang crashes because entry in
OffloadEntriesDeviceGlobalVar is unintialized. Current design supposes the
device compilation can only be invoked after host compilation with the host IR
such that clang can initialize OffloadEntriesDeviceGlobalVar from host IR.
This avoids us using device compilation directly, especially when we only have
code wrapped into declare target which are all device code. The same issue
also exists for OffloadEntriesInfoManager.

In this patch, we simply initialized an entry if it is not in the maps. Not sure
we need an option to tell the device compiler that it is invoked standalone.

[tianshilei1992]

Also updated for registerTargetRegionEntryInfo

[jdoerfert]

Here is a minimal reproducer for the issue even without standalone device compilation (and I can build the situation in various other ways as well): https://godbolt.org/z/T1h9b5
Can you add it as a test, I am also curious how the IR looks like with this patch.

[tianshilei1992]

Added a new test case for device only compilation

[ABataev]

clang/lib/CodeGen/CGOpenMPRuntime.cpp

2944–2948

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.

[jdoerfert]

LGTM, one nit in the test below.

clang/lib/CodeGen/CGOpenMPRuntime.cpp

2944–2948

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.

clang/test/OpenMP/declare_target_device_only_compilation.cpp

14

Run this as regular target offloading as well. no -fopenmp-is-device necessary, at least not in https://godbolt.org/z/T1h9b5.

[jdoerfert]

Oh, and fix the test. If you have -verify and no errors you need // expected-no-diagnostics

[ABataev]

clang/lib/CodeGen/CGOpenMPRuntime.cpp

2944–2948

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.

[tianshilei1992]

clang/lib/CodeGen/CGOpenMPRuntime.cpp

2944–2948

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.

[tianshilei1992]

Fixed the test case and rebased

[tianshilei1992]

Removed the useless assertion

[jdoerfert]

clang/lib/CodeGen/CGOpenMPRuntime.cpp

2944–2948

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.

[ABataev]

clang/lib/CodeGen/CGOpenMPRuntime.cpp

2944–2948

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?

[tianshilei1992]

clang/lib/CodeGen/CGOpenMPRuntime.cpp

2944–2948

Correct.

[jdoerfert]

clang/lib/CodeGen/CGOpenMPRuntime.cpp

2944–2948

FWIW, I can also use ifdef, or various other context selectors to achieve the same effect.

[ABataev]

clang/lib/CodeGen/CGOpenMPRuntime.cpp

2944–2948

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.

2944–2948

Ok, then go ahead with this solution.

[tianshilei1992]

clang/lib/CodeGen/CGOpenMPRuntime.cpp

2944–2948

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.

[ABataev]

clang/lib/CodeGen/CGOpenMPRuntime.cpp

2944–2948

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.

[JonChesterfield]

Nice! Thank you

clang/lib/CodeGen/CGOpenMPRuntime.cpp

 void CGOpenMPRuntime::OffloadEntriesInfoManagerTy::
     registerTargetRegionEntryInfo(unsigned DeviceID, unsigned FileID,
                                   StringRef ParentName, unsigned LineNum,
                                   llvm::Constant *Addr, llvm::Constant *ID,
                                   OMPTargetRegionEntryKind Flags) {
   // If we are emitting code for a target, the entry is already initialized,
   // only has to be registered.
   if (CGM.getLangOpts().OpenMPIsDevice) {
-    if (!hasTargetRegionEntryInfo(DeviceID, FileID, ParentName, LineNum)) {
-      unsigned DiagID = CGM.getDiags().getCustomDiagID(
-          DiagnosticsEngine::Error,
-          "Unable to find target region on line '%0' in the device code.");
-      CGM.getDiags().Report(DiagID) << LineNum;
-      return;
-    }
+    // This could happen if the device compilation is invoked standalone.
+    if (!hasTargetRegionEntryInfo(DeviceID, FileID, ParentName, LineNum))
+      initializeTargetRegionEntryInfo(DeviceID, FileID, ParentName, LineNum,
+                                      OffloadingEntriesNum);
     auto &Entry =
         OffloadEntriesTargetRegion[DeviceID][FileID][ParentName][LineNum];
-    assert(Entry.isValid() && "Entry not initialized!");
     Entry.setAddress(Addr);
     Entry.setID(ID);
     Entry.setFlags(Flags);
   } else {
     if (Flags ==
             OffloadEntriesInfoManagerTy::OMPTargetRegionEntryTargetRegion &&
         hasTargetRegionEntryInfo(DeviceID, FileID, ParentName, LineNum,
                                  /*IgnoreAddressId*/ true))

 }
 
 void CGOpenMPRuntime::OffloadEntriesInfoManagerTy::
     registerDeviceGlobalVarEntryInfo(StringRef VarName, llvm::Constant *Addr,
                                      CharUnits VarSize,
                                      OMPTargetGlobalVarEntryKind Flags,
                                      llvm::GlobalValue::LinkageTypes Linkage) {
   if (CGM.getLangOpts().OpenMPIsDevice) {
+    // This could happen if the device compilation is invoked standalone.
+    if (!hasDeviceGlobalVarEntryInfo(VarName))
+      initializeDeviceGlobalVarEntryInfo(VarName, Flags, OffloadingEntriesNum);
     auto &Entry = OffloadEntriesDeviceGlobalVar[VarName];
-    assert(Entry.isValid() && Entry.getFlags() == Flags &&
-           "Entry not initialized!");
     assert((!Entry.getAddress() || Entry.getAddress() == Addr) &&
            "Resetting with the new address.");
     if (Entry.getAddress() && hasDeviceGlobalVarEntryInfo(VarName)) {
       if (Entry.getVarSize().isZero()) {
         Entry.setVarSize(VarSize);
         Entry.setLinkage(Linkage);
       }
       return;

clang/test/OpenMP/declare_target_device_only_compilation.cpp

+//==========================================================================///
+// RUN: %clang -S -target powerpc64le-ibm-linux-gnu -fopenmp -fopenmp-targets=powerpc64le-ibm-linux-gnu -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang -S -target i386-pc-linux-gnu -fopenmp -fopenmp-targets=i386-pc-linux-gnu -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang -S -target x86_64-unknown-linux-gnu -fopenmp -fopenmp-targets=x86_64-unknown-linux-gnu -emit-llvm %s -o - | FileCheck %s
+// expected-no-diagnostics
+
+#pragma omp declare target
+#pragma omp begin declare variant match(device={kind(nohost)})
+int G1;
+#pragma omp end declare variant
+#pragma omp end declare target
+
+// CHECK: @[[G:.+]] = hidden {{.*}}global i32 0, align 4
+// CHECK: !omp_offload.info = !{!0}
+// CHECK: !0 = !{i32 1, !"[[G]]", i32 0, i32 0}