[SYCL][Clang] Permit virtual functions in SYCL

[KornevNikita]

To enable virtual functions support in SYCL we need to change virtual table variables address space, so llvm-spirv translator is able to generate valid SPIR-V modules.

[KornevNikita]

@intel/dpcpp-cfe-reviewers could you please take a look? It's a draft because there is no test and lot of lit-test failures. I will add a test and fix failures if this patch is valid.

[premanandrao]

Quick review.

[KornevNikita]

E2E is in the progress

[mdtoguchi]

OK for driver

[elizabethandrews]

Please add a test.

[elizabethandrews]

VTableTy is Int8PtrTy for both SYCL and non-SYCL case above right? So what is the else for?

[KornevNikita]

I believe Int8PtrTy and Int8PtrTy(LangAS::opencl_global) are different, am I wrong?

[elizabethandrews]

I am not sure but in any case, I think it is better to guard this using SYCLIsDevice.

I am not familiar with this code so I may be wrong but do we need the call back function? Won't global variable be constructed with modified VTableTy without the callback back? Or do default constructors not consider address space?

[Fznamznon]

I think all new changes need to be guarded with SYCLIsDevice if as well as SYCLAllowVirtualFunctions.

[Fznamznon]

Smth like

if (!VTable) {
  Type *VTableTy = CGM.Int8PtrTy;
  if (CGM.getLangOpts().SYCLIsDevice &&
      CGM.getLangOpts().SYCLAllowVirtualFunctions &&
      (VTableName == ClassTypeInfo || VTableName == SIClassTypeInfo)) {
    VTableTy =
        CGM.Int8Ty->getPointerTo(static_cast<unsigned>(LangAS::opencl_global))
            VTable =
            CGM.getModule().getOrInsertGlobal(VTableName, VTableTy, [&] {
              return new llvm::GlobalVariable(
                  CGM.getModule(), VTableTy, false,
                  llvm::GlobalVariable::ExternalLinkage, nullptr, VTableName,
                  nullptr, llvm::GlobalValue::ThreadLocalMode::NotThreadLocal,
                  llvm::Optional<unsigned>(
                      static_cast<unsigned>(LangAS::opencl_global)));
            });
  } else {
    VTable = CGM.getModule().getOrInsertGlobal(VTableName, VTableTy);
  }
}

[KornevNikita]

Are you ok with c810c23 or you don't want to see VTableTy with opencl_global outside of this guards?

[Fznamznon]

I think all new changes need to be guarded with SYCLIsDevice if as well as SYCLAllowVirtualFunctions.

[Fznamznon]

Smth like

if (!VTable) {
  Type *VTableTy = CGM.Int8PtrTy;
  if (CGM.getLangOpts().SYCLIsDevice &&
      CGM.getLangOpts().SYCLAllowVirtualFunctions &&
      (VTableName == ClassTypeInfo || VTableName == SIClassTypeInfo)) {
    VTableTy =
        CGM.Int8Ty->getPointerTo(static_cast<unsigned>(LangAS::opencl_global))
            VTable =
            CGM.getModule().getOrInsertGlobal(VTableName, VTableTy, [&] {
              return new llvm::GlobalVariable(
                  CGM.getModule(), VTableTy, false,
                  llvm::GlobalVariable::ExternalLinkage, nullptr, VTableName,
                  nullptr, llvm::GlobalValue::ThreadLocalMode::NotThreadLocal,
                  llvm::Optional<unsigned>(
                      static_cast<unsigned>(LangAS::opencl_global)));
            });
  } else {
    VTable = CGM.getModule().getOrInsertGlobal(VTableName, VTableTy);
  }
}

[Fznamznon]

Also, I believe @elizabethandrews means clang LIT test. E2E test only won't be enough.

[KornevNikita]

@intel/llvm-gatekeepers

[steffenlarsen]

Failures unrelated:

• SYCL :: Regression/device_num.cpp - Fix in [SYCL] Allow ESIMD backend only when explicitly set by ONEAPI_DEVICE_SELECTOR #7336.
• SYCL :: regression/tanh_fix_test.cpp - Fix in [SYCL][ESIMD] Make the tanh test pass in ESIMD Emulator llvm-test-suite#1372.
• SYCL :: Basic/barrier_order.cpp - Reported in Basic/barrier_order.cpp hangs on opencl:gpu #7330. Temporarily disabled in [SYCL] Temporarily disable barrier_order for HIP and L0 backends llvm-test-suite#1376.