[CLANG] Fix missing error for use of 128-bit integer inside SPIR64 device code.

[jyu2]

Summary

Emit error for use of 128-bit integer inside device code had been
already implemented in https://reviews.llvm.org/D74387. However,
the error is not emitted for SPIR64, due for SPIR64 hasInt128Type
return true.

hasInt128Type: is also used to control generation of certain 128-bit
predefined macros, initializer predefined 128-bit integer types and
build 128-bit ArithmeticTypes. Except predefined macros, only the
device target is considered, since error only emit when 128-bit
integer is used inside device code, the host target (auxtarget) also
needs to be considered.

The change address:

1. (SPIR.h) Correct hasInt128Type() for SPIR targets.
2. Sema.cpp and SemaOverload.cpp: Add additional check to consider host target(auxtarget) when call to hasInt128Type. So that int128_t and int128() are allowed to avoid error when they used outside device code.
3. SemaType.cpp: add check for SYCLIsDevice to delay the error message. The error will be emitted if the use of 128-bit integer in the device code.

[jdoerfert]

clang/lib/Basic/Targets/SPIR.h

108

Not virtual but override.

clang/lib/Sema/Sema.cpp

241

I don't understand why this (and the changes below) are necessary. Host and device compilation are separate. This should not be any different to CUDA, HIP, or OpenMP offload which seem not to require this.

[jyu2]

clang/lib/Basic/Targets/SPIR.h

108

Thank you. I will change that.

clang/lib/Sema/Sema.cpp

241

As far as I can know, in real compile, the auxtarget should be also set for example SYCL. May be that is only for our compiler. I am not sure what do you mean by it should same with CUDA, HIP...? Do you mean, CUDA(or HIP...) target should also not support 128-bit integer? If so, I don't see any error emit for CUDA when I try with nvptx64-nvidia-cuda.

Thanks.

[jdoerfert]

So, NVPTX seems to do something sensible with i128, though I haven't done rigorous testing.

clang/lib/Sema/Sema.cpp

241

I'm not saying auxtarget is not set. I'm trying to understand why this is necessary. Why would you initialize __int128_t if your target doesn't support it? What happens if you only include the SPIR.h change?

[jyu2]

clang/lib/Sema/Sema.cpp

241

Without this change you will see error:

j3.c:2:15: error: unknown type name 'uint128_t'
typedef const uint128_t megeType;

bash-4.4$ cat j3.c

typedef const __uint128_t megeType;

Without change in SemaOverload.c:
you will see error:
x.cpp:3:14: error: use of overloaded operator '==' is ambiguous (with operand types 'struct X' and '__int128')

bool a = x == __int128(0);
         ~ ^  ~~~~~~~~~~~

bash-4.4$ cat x.cpp
namespace PR12964 {

struct X { operator  __int128() const; } x;
bool a = x == __int128(0);

}

[jyu2]

clang/lib/Sema/Sema.cpp

241

Just one more point, the errors only need to be emitted for 128-bit integer used inside device code. The test case above 128-bit integer is not used in device code, so we don't want to emit error for it.

Thanks.
Jennifer

[jdoerfert]

Still unsure if we should also error out for NVPTX but that is a different story. Looks OK from my side, assuming you address the earlier comment.

Maybe someone else should accept though.

clang/lib/Sema/Sema.cpp

241

I see. You want the types to be there so the diagnostic says the types are not available. OK, fine by me.

[jyu2]

Fix problem with @jdoerfert pointed where using bool hasInt128Type() const override { return false; } instead.

[jyu2]

clang/lib/Sema/Sema.cpp

241

Thank you so much for your time for review this!!!

[jyu2]

In D92439#2429511, @jdoerfert wrote:

Still unsure if we should also error out for NVPTX but that is a different story. Looks OK from my side, assuming you address the earlier comment.

With this change if NVPTX need diagnostic for use of 128-bit integer, adding "bool hasInt128Type() const override { return false; }" in NVPTX.h is all needed.

Maybe someone else should accept though.

Do you have suggestion whom I may contact for acceptance? We have customer needs for this... Thank you in advance. :-)

[jdoerfert]

In D92439#2429815, @jyu2 wrote:

In D92439#2429511, @jdoerfert wrote:

Still unsure if we should also error out for NVPTX but that is a different story. Looks OK from my side, assuming you address the earlier comment.

With this change if NVPTX need diagnostic for use of 128-bit integer, adding "bool hasInt128Type() const override { return false; }" in NVPTX.h is all needed.

Maybe someone else should accept though.

Do you have suggestion whom I may contact for acceptance? We have customer needs for this... Thank you in advance. :-)

You have the right people as reviewers, it sometimes need a few days. You can ping it after a week without progress.

[aaron.ballman]

In D92439#2431256, @jdoerfert wrote:

In D92439#2429815, @jyu2 wrote:

In D92439#2429511, @jdoerfert wrote:

Still unsure if we should also error out for NVPTX but that is a different story. Looks OK from my side, assuming you address the earlier comment.

With this change if NVPTX need diagnostic for use of 128-bit integer, adding "bool hasInt128Type() const override { return false; }" in NVPTX.h is all needed.

Maybe someone else should accept though.

Do you have suggestion whom I may contact for acceptance? We have customer needs for this... Thank you in advance. :-)

You have the right people as reviewers, it sometimes need a few days. You can ping it after a week without progress.

FWIW, the changes LGTM but I don't know enough about the domain to know the answer for NVPTX. That said, this is still good incremental progress as-is.

[jdoerfert]

In D92439#2431270, @aaron.ballman wrote:

In D92439#2431256, @jdoerfert wrote:

In D92439#2429815, @jyu2 wrote:

In D92439#2429511, @jdoerfert wrote:

Still unsure if we should also error out for NVPTX but that is a different story. Looks OK from my side, assuming you address the earlier comment.

With this change if NVPTX need diagnostic for use of 128-bit integer, adding "bool hasInt128Type() const override { return false; }" in NVPTX.h is all needed.

Maybe someone else should accept though.

Do you have suggestion whom I may contact for acceptance? We have customer needs for this... Thank you in advance. :-)

You have the right people as reviewers, it sometimes need a few days. You can ping it after a week without progress.

FWIW, the changes LGTM but I don't know enough about the domain to know the answer for NVPTX. That said, this is still good incremental progress as-is.

Leave NVPTX out for now. As I said, it looks like the backend does the right thing so it is "legal" to have i128.

[jyu2]

Thanks @aaron.ballman to accept my change!!!
Hi @jdoerfert,
Thank you for your review.
it seems you still has request change for for this. I did address your earlier comment in SPIR.h and upload the change. Is there anything else I need to change?

Thanks.
Jennifer

[jdoerfert]

LG

[jyu2]

commit by rGf8d5b49c786f: Fix missing error for use of 128-bit integer inside SPIR64 device code.

clang/lib/Basic/Targets/SPIR.h

public:
 
   void setSupportedOpenCLOpts() override {
     // Assume all OpenCL extensions and optional core features are supported
     // for SPIR since it is a generic target.
     getSupportedOpenCLOpts().supportAll();
   }
 
   bool hasExtIntType() const override { return true; }
+
+  bool hasInt128Type() const override { return false; }
 };
 class LLVM_LIBRARY_VISIBILITY SPIR32TargetInfo : public SPIRTargetInfo {
 public:
   SPIR32TargetInfo(const llvm::Triple &Triple, const TargetOptions &Opts)
       : SPIRTargetInfo(Triple, Opts) {
     PointerWidth = PointerAlign = 32;
     SizeType = TargetInfo::UnsignedInt;
     PtrDiffType = IntPtrType = TargetInfo::SignedInt;

clang/lib/Sema/Sema.cpp

void Sema::Initialize() {
   // This needs to happen after ExternalSemaSource::InitializeSema(this) or we
   // will not be able to merge any duplicate __va_list_tag decls correctly.
   VAListTagName = PP.getIdentifierInfo("__va_list_tag");
 
   if (!TUScope)
     return;
 
   // Initialize predefined 128-bit integer types, if needed.
-  if (Context.getTargetInfo().hasInt128Type()) {
+  if (Context.getTargetInfo().hasInt128Type() ||
+      (Context.getAuxTargetInfo() &&
+       Context.getAuxTargetInfo()->hasInt128Type())) {
     // If either of the 128-bit integer types are unavailable to name lookup,
     // define them now.
     DeclarationName Int128 = &Context.Idents.get("__int128_t");
     if (IdResolver.begin(Int128) == IdResolver.end())
       PushOnScopeChains(Context.getInt128Decl(), TUScope);
 
     DeclarationName UInt128 = &Context.Idents.get("__uint128_t");
     if (IdResolver.begin(UInt128) == IdResolver.end())

clang/lib/Sema/SemaOverload.cpp

if (S.Context.getTargetInfo().hasFloat128Type())
       ArithmeticTypes.push_back(S.Context.Float128Ty);
 
     // Start of integral types.
     FirstIntegralType = ArithmeticTypes.size();
     FirstPromotedIntegralType = ArithmeticTypes.size();
     ArithmeticTypes.push_back(S.Context.IntTy);
     ArithmeticTypes.push_back(S.Context.LongTy);
     ArithmeticTypes.push_back(S.Context.LongLongTy);
-    if (S.Context.getTargetInfo().hasInt128Type())
+    if (S.Context.getTargetInfo().hasInt128Type() ||
+        (S.Context.getAuxTargetInfo() &&
+         S.Context.getAuxTargetInfo()->hasInt128Type()))
       ArithmeticTypes.push_back(S.Context.Int128Ty);
     ArithmeticTypes.push_back(S.Context.UnsignedIntTy);
     ArithmeticTypes.push_back(S.Context.UnsignedLongTy);
     ArithmeticTypes.push_back(S.Context.UnsignedLongLongTy);
-    if (S.Context.getTargetInfo().hasInt128Type())
+    if (S.Context.getTargetInfo().hasInt128Type() ||
+        (S.Context.getAuxTargetInfo() &&
+         S.Context.getAuxTargetInfo()->hasInt128Type()))
       ArithmeticTypes.push_back(S.Context.UnsignedInt128Ty);
     LastPromotedIntegralType = ArithmeticTypes.size();
     LastPromotedArithmeticType = ArithmeticTypes.size();
     // End of promoted types.
 
     ArithmeticTypes.push_back(S.Context.BoolTy);
     ArithmeticTypes.push_back(S.Context.CharTy);
     ArithmeticTypes.push_back(S.Context.WCharTy);

clang/lib/Sema/SemaType.cpp

case DeclSpec::TST_fract: {
 
     if (DS.isTypeSpecSat())
       Result = Context.getCorrespondingSaturatedType(Result);
 
     break;
   }
   case DeclSpec::TST_int128:
     if (!S.Context.getTargetInfo().hasInt128Type() &&
+        !S.getLangOpts().SYCLIsDevice &&
         !(S.getLangOpts().OpenMP && S.getLangOpts().OpenMPIsDevice))
       S.Diag(DS.getTypeSpecTypeLoc(), diag::err_type_unsupported)
         << "__int128";
     if (DS.getTypeSpecSign() == TypeSpecifierSign::Unsigned)
       Result = Context.UnsignedInt128Ty;
     else
       Result = Context.Int128Ty;
     break;

clang/test/CodeGen/ext-int-cc.c

 // WIN32: define dso_local void @ParamPassing(i129* %{{.+}}, i128* %{{.+}}, i64 %{{.+}})
 // NACL: define void @ParamPassing(i129* byval(i129) align 8 %{{.+}}, i128* byval(i128) align 8 %{{.+}}, i64 %{{.+}})
 // NVPTX64: define void @ParamPassing(i129* byval(i129) align 8 %{{.+}}, i128 %{{.+}}, i64 %{{.+}})
 // NVPTX: define void @ParamPassing(i129* byval(i129) align 8 %{{.+}}, i128* byval(i128) align 8 %{{.+}}, i64 %{{.+}})
 // SPARCV9: define void @ParamPassing(i129* %{{.+}}, i128 %{{.+}}, i64 %{{.+}})
 // SPARC: define void @ParamPassing(i129* byval(i129) align 8 %{{.+}}, i128* byval(i128) align 8 %{{.+}}, i64 %{{.+}})
 // MIPS64: define void @ParamPassing(i129* byval(i129) align 8 %{{.+}}, i128 signext  %{{.+}}, i64 signext %{{.+}})
 // MIPS: define void @ParamPassing(i129* byval(i129) align 8 %{{.+}}, i128* byval(i128) align 8 %{{.+}}, i64 signext %{{.+}})
-// SPIR64: define spir_func void @ParamPassing(i129* byval(i129) align 8 %{{.+}}, i128 %{{.+}}, i64 %{{.+}})
+// SPIR64: define spir_func void @ParamPassing(i129* byval(i129) align 8 %{{.+}}, i128* byval(i128) align 8 %{{.+}}, i64 %{{.+}})
 // SPIR: define spir_func void @ParamPassing(i129* byval(i129) align 8 %{{.+}}, i128* byval(i128) align 8 %{{.+}}, i64 %{{.+}})
 // HEX: define void @ParamPassing(i129* byval(i129) align 8 %{{.+}}, i128* byval(i128) align 8 %{{.+}}, i64 %{{.+}})
 // LANAI: define void @ParamPassing(i129* byval(i129) align 4 %{{.+}}, i128* byval(i128) align 4 %{{.+}}, i64 %{{.+}})
 // R600: define void @ParamPassing(i129 addrspace(5)* byval(i129) align 8 %{{.+}}, i128 addrspace(5)* byval(i128) align 8 %{{.+}}, i64 %{{.+}})
 // ARC: define void @ParamPassing(i129* byval(i129) align 4 %{{.+}}, i128* byval(i128) align 4 %{{.+}}, i64 inreg %{{.+}})
 // XCORE: define void @ParamPassing(i129* byval(i129) align 4 %{{.+}}, i128* byval(i128) align 4 %{{.+}}, i64 %{{.+}})
 // RISCV64: define void @ParamPassing(i129* %{{.+}}, i128 %{{.+}}, i64 %{{.+}})
 // RISCV32: define void @ParamPassing(i129* %{{.+}}, i128* %{{.+}}, i64 %{{.+}})

 // WIN32: define dso_local void @ParamPassing2(i129* %{{.+}}, i127* %{{.+}}, i63 %{{.+}})
 // NACL: define void @ParamPassing2(i129* byval(i129) align 8 %{{.+}}, i127* byval(i127) align 8 %{{.+}}, i63 %{{.+}})
 // NVPTX64: define void @ParamPassing2(i129* byval(i129) align 8 %{{.+}}, i127 %{{.+}}, i63 %{{.+}})
 // NVPTX: define void @ParamPassing2(i129* byval(i129) align 8 %{{.+}}, i127* byval(i127) align 8 %{{.+}}, i63 %{{.+}})
 // SPARCV9: define void @ParamPassing2(i129* %{{.+}}, i127 %{{.+}}, i63 signext %{{.+}})
 // SPARC: define void @ParamPassing2(i129* byval(i129) align 8 %{{.+}}, i127* byval(i127) align 8 %{{.+}}, i63 %{{.+}})
 // MIPS64: define void @ParamPassing2(i129* byval(i129) align 8 %{{.+}}, i127 signext  %{{.+}}, i63 signext %{{.+}})
 // MIPS: define void @ParamPassing2(i129* byval(i129) align 8 %{{.+}}, i127* byval(i127) align 8 %{{.+}}, i63 signext %{{.+}})
-// SPIR64: define spir_func void @ParamPassing2(i129* byval(i129) align 8 %{{.+}}, i127 %{{.+}}, i63 %{{.+}})
+// SPIR64: define spir_func void @ParamPassing2(i129* byval(i129) align 8 %{{.+}}, i127* byval(i127) align 8 %{{.+}}, i63 %{{.+}})
 // SPIR: define spir_func void @ParamPassing2(i129* byval(i129) align 8 %{{.+}}, i127* byval(i127) align 8 %{{.+}}, i63 %{{.+}})
 // HEX: define void @ParamPassing2(i129* byval(i129) align 8 %{{.+}}, i127* byval(i127) align 8 %{{.+}}, i63 %{{.+}})
 // LANAI: define void @ParamPassing2(i129* byval(i129) align 4 %{{.+}}, i127* byval(i127) align 4 %{{.+}}, i63 %{{.+}})
 // R600: define void @ParamPassing2(i129 addrspace(5)* byval(i129) align 8 %{{.+}}, i127 addrspace(5)* byval(i127) align 8 %{{.+}}, i63 %{{.+}})
 // ARC: define void @ParamPassing2(i129* byval(i129) align 4 %{{.+}}, i127* byval(i127) align 4 %{{.+}}, i63 inreg %{{.+}})
 // XCORE: define void @ParamPassing2(i129* byval(i129) align 4 %{{.+}}, i127* byval(i127) align 4 %{{.+}}, i63 %{{.+}})
 // RISCV64: define void @ParamPassing2(i129* %{{.+}}, i127 %{{.+}}, i63 signext %{{.+}})
 // RISCV32: define void @ParamPassing2(i129* %{{.+}}, i127* %{{.+}}, i63 %{{.+}})

 // NVPTX/64 makes the intentional choice to put all return values direct, even
 // large structures, so we do the same here.
 // NVPTX64: define i127 @ReturnPassing3(
 // NVPTX: define i127 @ReturnPassing3(
 // SPARCV9: define i127 @ReturnPassing3(
 // SPARC: define void @ReturnPassing3(i127* noalias sret
 // MIPS64: define i127 @ReturnPassing3(
 // MIPS: define void @ReturnPassing3(i127* noalias sret
-// SPIR64: define spir_func i127 @ReturnPassing3(
+// SPIR64: define spir_func void @ReturnPassing3(i127* noalias sret
 // SPIR: define spir_func void @ReturnPassing3(i127* noalias sret
 // HEX: define void @ReturnPassing3(i127* noalias sret
 // LANAI: define void @ReturnPassing3(i127* noalias sret
 // R600: define void @ReturnPassing3(i127 addrspace(5)* noalias sret
 // ARC: define void @ReturnPassing3(i127* noalias sret
 // XCORE: define void @ReturnPassing3(i127* noalias sret
 // RISCV64: define i127 @ReturnPassing3(
 // RISCV32: define void @ReturnPassing3(i127* noalias sret

 // WIN32: define dso_local void @ReturnPassing4(i128* noalias sret
 // NACL: define void @ReturnPassing4(i128* noalias sret
 // NVPTX64: define i128 @ReturnPassing4(
 // NVPTX: define i128 @ReturnPassing4(
 // SPARCV9: define i128 @ReturnPassing4(
 // SPARC: define void @ReturnPassing4(i128* noalias sret
 // MIPS64: define i128 @ReturnPassing4(
 // MIPS: define void @ReturnPassing4(i128* noalias sret
-// SPIR64: define spir_func i128 @ReturnPassing4(
+// SPIR64: define spir_func void @ReturnPassing4(i128* noalias sret
 // SPIR: define spir_func void @ReturnPassing4(i128* noalias sret
 // HEX: define void @ReturnPassing4(i128* noalias sret
 // LANAI: define void @ReturnPassing4(i128* noalias sret
 // R600: define void @ReturnPassing4(i128 addrspace(5)* noalias sret
 // ARC: define void @ReturnPassing4(i128* noalias sret
 // XCORE: define void @ReturnPassing4(i128* noalias sret
 // RISCV64: define i128 @ReturnPassing4(
 // RISCV32: define void @ReturnPassing4(i128* noalias sret

clang/test/SemaSYCL/int128.cpp

+// RUN: %clang_cc1 -triple spir64 -aux-triple x86_64-unknown-linux-gnu \
+// RUN:    -fsycl -fsycl-is-device -verify -fsyntax-only %s
+
+typedef __uint128_t BIGTY;
+
+template <class T>
+class Z {
+public:
+  // expected-note@+1 {{'field' defined here}}
+  T field;
+  // expected-note@+1 2{{'field1' defined here}}
+  __int128 field1;
+  using BIGTYPE = __int128;
+  // expected-note@+1 {{'bigfield' defined here}}
+  BIGTYPE bigfield;
+};
+
+void host_ok(void) {
+  __int128 A;
+  int B = sizeof(__int128);
+  Z<__int128> C;
+  C.field1 = A;
+}
+
+void usage() {
+  // expected-note@+1 3{{'A' defined here}}
+  __int128 A;
+  Z<__int128> C;
+  // expected-error@+2 {{'A' requires 128 bit size '__int128' type support, but device 'spir64' does not support it}}
+  // expected-error@+1 {{'field1' requires 128 bit size '__int128' type support, but device 'spir64' does not support it}}
+  C.field1 = A;
+  // expected-error@+1 {{'bigfield' requires 128 bit size 'Z::BIGTYPE' (aka '__int128') type support, but device 'spir64' does not support it}}
+  C.bigfield += 1.0;
+
+  // expected-error@+1 {{'A' requires 128 bit size '__int128' type support, but device 'spir64' does not support it}}
+  auto foo1 = [=]() {
+    __int128 AA;
+    // expected-note@+2 {{'BB' defined here}}
+    // expected-error@+1 {{'A' requires 128 bit size '__int128' type support, but device 'spir64' does not support it}}
+    auto BB = A;
+    // expected-error@+1 {{'BB' requires 128 bit size '__int128' type support, but device 'spir64' does not support it}}
+    BB += 1;
+  };
+
+  // expected-note@+1 {{called by 'usage'}}
+  foo1();
+}
+
+template <typename t>
+void foo2(){};
+
+// expected-note@+3 {{'P' defined here}}
+// expected-error@+2 {{'P' requires 128 bit size '__int128' type support, but device 'spir64' does not support it}}
+// expected-note@+1 2{{'foo' defined here}}
+__int128 foo(__int128 P) { return P; }
+
+void foobar() {
+  // expected-note@+1 {{'operator __int128' defined here}}
+  struct X { operator  __int128() const; } x;
+  bool a = false;
+  // expected-error@+1 {{'operator __int128' requires 128 bit size '__int128' type support, but device 'spir64' does not support it}}
+  a = x == __int128(0);
+}
+
+template <typename Name, typename Func>
+__attribute__((sycl_kernel)) void kernel(Func kernelFunc) {
+  // expected-note@+1 6{{called by 'kernel}}
+  kernelFunc();
+}
+
+int main() {
+  // expected-note@+1 {{'CapturedToDevice' defined here}}
+  __int128 CapturedToDevice = 1;
+  host_ok();
+  kernel<class variables>([=]() {
+    decltype(CapturedToDevice) D;
+    // expected-error@+1 {{'CapturedToDevice' requires 128 bit size '__int128' type support, but device 'spir64' does not support it}}
+    auto C = CapturedToDevice;
+    Z<__int128> S;
+    // expected-error@+1 {{'field1' requires 128 bit size '__int128' type support, but device 'spir64' does not support it}}
+    S.field1 += 1;
+    // expected-error@+1 {{'field' requires 128 bit size '__int128' type support, but device 'spir64' does not support it}}
+    S.field = 1;
+  });
+
+  kernel<class functions>([=]() {
+    // expected-note@+1 2{{called by 'operator()'}}
+    usage();
+    // expected-note@+1 {{'BBBB' defined here}}
+    BIGTY BBBB;
+    // expected-error@+3 {{'BBBB' requires 128 bit size 'BIGTY' (aka 'unsigned __int128') type support, but device 'spir64' does not support it}}
+    // expected-error@+2 2{{'foo' requires 128 bit size '__int128' type support, but device 'spir64' does not support it}}
+    // expected-note@+1 1{{called by 'operator()'}}
+    auto A = foo(BBBB);
+    // expected-note@+1 {{called by 'operator()'}}
+    foobar();
+  });
+
+  kernel<class ok>([=]() {
+    Z<__int128> S;
+    foo2<__int128>();
+    auto A = sizeof(CapturedToDevice);
+  });
+
+  return 0;
+}
+
+// no error expected
+BIGTY zoo(BIGTY h) {
+  h = 1;
+  return h;
+}
+
+namespace PR12964 {
+  struct X { operator  __int128() const; } x;
+  bool a = x == __int128(0);
+}
+