[PATCH] D141375: [SYCL][OpenMP] Fix compilation errors for unsupported __bf16 intrinsics

[Artem Belevich via Phabricator via cfe-commits]

tra added inline comments.



Comment at: clang/lib/Sema/Sema.cpp:1978-1979
  !Context.getTargetInfo().hasInt128Type()) ||
+(Ty->isBFloat16Type() && !Context.getTargetInfo().hasBFloat16Type() &&
+ !LangOpts.CUDAIsDevice) ||
 LongDoubleMismatched) {

eandrews wrote:
> tra wrote:
> > @eandrews Do you recall what was the reason for *not* issuing the 
> > diagnostic on the GPU side?
> > 
> > It appears to do the opposite to what the patch description says. We're 
> > supposed to  `emit error for unsupported type __bf16 in device code`, but 
> > instead we specifically ignore it during GPU-side compilation. What am I 
> > missing?
> > 
> > 
> > 
> I don't recall the specifics but I think CUDA had code handling __bf16 
> differently and this change broke a test with CUDA diagnostics and so I 
> excluded it from the patch. I could try removing this check and seeing what 
> breaks if you'd like. 
It may have been around the time when x86 started exposing bf16 type in the 
host headers, but NVPTX didn't have any support for the type yet.

This change may have just papered over the problem. Oh, well. That would be 
just one of the places where we currently don't handle the 'unusual' types 
across the host/GPU boundary. I'm attempting to clean it up, and will take care 
of this instance there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141375

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141375: [SYCL][OpenMP] Fix compilation errors for unsupported __bf16 intrinsics

[Elizabeth Andrews via Phabricator via cfe-commits]

eandrews added inline comments.



Comment at: clang/lib/Sema/Sema.cpp:1978-1979
  !Context.getTargetInfo().hasInt128Type()) ||
+(Ty->isBFloat16Type() && !Context.getTargetInfo().hasBFloat16Type() &&
+ !LangOpts.CUDAIsDevice) ||
 LongDoubleMismatched) {

tra wrote:
> @eandrews Do you recall what was the reason for *not* issuing the diagnostic 
> on the GPU side?
> 
> It appears to do the opposite to what the patch description says. We're 
> supposed to  `emit error for unsupported type __bf16 in device code`, but 
> instead we specifically ignore it during GPU-side compilation. What am I 
> missing?
> 
> 
> 
I don't recall the specifics but I think CUDA had code handling __bf16 
differently and this change broke a test with CUDA diagnostics and so I 
excluded it from the patch. I could try removing this check and seeing what 
breaks if you'd like. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141375

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141375: [SYCL][OpenMP] Fix compilation errors for unsupported __bf16 intrinsics

[Artem Belevich via Phabricator via cfe-commits]

tra added inline comments.
Herald added a subscriber: jplehr.



Comment at: clang/lib/Sema/Sema.cpp:1978-1979
  !Context.getTargetInfo().hasInt128Type()) ||
+(Ty->isBFloat16Type() && !Context.getTargetInfo().hasBFloat16Type() &&
+ !LangOpts.CUDAIsDevice) ||
 LongDoubleMismatched) {

@eandrews Do you recall what was the reason for *not* issuing the diagnostic on 
the GPU side?

It appears to do the opposite to what the patch description says. We're 
supposed to  `emit error for unsupported type __bf16 in device code`, but 
instead we specifically ignore it during GPU-side compilation. What am I 
missing?





Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141375

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141375: [SYCL][OpenMP] Fix compilation errors for unsupported __bf16 intrinsics

[Elizabeth Andrews via Phabricator via cfe-commits]

eandrews added a comment.

Thanks for the reviews!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141375

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141375: [SYCL][OpenMP] Fix compilation errors for unsupported __bf16 intrinsics

[Elizabeth Andrews via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGf81d529f8955: [Clang] Fix compilation errors for unsupported 
__bf16 intrinsics (authored by eandrews).
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141375

Files:
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/ItaniumMangle.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/SemaSYCL/bf16.cpp


Index: clang/test/SemaSYCL/bf16.cpp
===
--- /dev/null
+++ clang/test/SemaSYCL/bf16.cpp
@@ -0,0 +1,22 @@
+// RUN: %clang_cc1 -triple spir64 -aux-triple x86_64-unknown-linux-gnu 
-fsycl-is-device -verify -fsyntax-only %s
+
+template 
+__attribute__((sycl_kernel)) void kernel(Func kernelFunc) {
+  kernelFunc(); // expected-note {{called by 'kernel}}
+}
+
+void host_ok(void) {
+  __bf16 A;
+}
+
+int main()
+{  host_ok();
+  __bf16 var; // expected-note {{'var' defined here}}
+  kernel([=]() {
+(void)var; // expected-error {{'var' requires 16 bit size '__bf16' type 
support, but target 'spir64' does not support it}}
+int B = sizeof(__bf16);
+  });
+
+  return 0;
+}
+
Index: clang/lib/Sema/SemaType.cpp
===
--- clang/lib/Sema/SemaType.cpp
+++ clang/lib/Sema/SemaType.cpp
@@ -1518,9 +1518,10 @@
 break;
   case DeclSpec::TST_half:Result = Context.HalfTy; break;
   case DeclSpec::TST_BFloat16:
-if (!S.Context.getTargetInfo().hasBFloat16Type())
-  S.Diag(DS.getTypeSpecTypeLoc(), diag::err_type_unsupported)
-<< "__bf16";
+if (!S.Context.getTargetInfo().hasBFloat16Type() &&
+!(S.getLangOpts().OpenMP && S.getLangOpts().OpenMPIsDevice) &&
+!S.getLangOpts().SYCLIsDevice)
+  S.Diag(DS.getTypeSpecTypeLoc(), diag::err_type_unsupported) << "__bf16";
 Result = Context.BFloat16Ty;
 break;
   case DeclSpec::TST_float:   Result = Context.FloatTy; break;
Index: clang/lib/Sema/Sema.cpp
===
--- clang/lib/Sema/Sema.cpp
+++ clang/lib/Sema/Sema.cpp
@@ -1975,6 +1975,8 @@
 (Ty->isIbm128Type() && !Context.getTargetInfo().hasIbm128Type()) ||
 (Ty->isIntegerType() && Context.getTypeSize(Ty) == 128 &&
  !Context.getTargetInfo().hasInt128Type()) ||
+(Ty->isBFloat16Type() && !Context.getTargetInfo().hasBFloat16Type() &&
+ !LangOpts.CUDAIsDevice) ||
 LongDoubleMismatched) {
   PartialDiagnostic PD = PDiag(diag::err_target_unsupported_type);
   if (D)
Index: clang/lib/AST/ItaniumMangle.cpp
===
--- clang/lib/AST/ItaniumMangle.cpp
+++ clang/lib/AST/ItaniumMangle.cpp
@@ -3051,7 +3051,11 @@
 break;
   }
   case BuiltinType::BFloat16: {
-const TargetInfo *TI = ().getTargetInfo();
+const TargetInfo *TI = ((getASTContext().getLangOpts().OpenMP &&
+ getASTContext().getLangOpts().OpenMPIsDevice) ||
+getASTContext().getLangOpts().SYCLIsDevice)
+   ? getASTContext().getAuxTargetInfo()
+   : ().getTargetInfo();
 Out << TI->getBFloat16Mangling();
 break;
   }
Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -2140,6 +2140,11 @@
   if (Target->hasBFloat16Type()) {
 Width = Target->getBFloat16Width();
 Align = Target->getBFloat16Align();
+  } else if ((getLangOpts().SYCLIsDevice ||
+  (getLangOpts().OpenMP && getLangOpts().OpenMPIsDevice)) &&
+ AuxTarget->hasBFloat16Type()) {
+Width = AuxTarget->getBFloat16Width();
+Align = AuxTarget->getBFloat16Align();
   }
   break;
 case BuiltinType::Float16:


Index: clang/test/SemaSYCL/bf16.cpp
===
--- /dev/null
+++ clang/test/SemaSYCL/bf16.cpp
@@ -0,0 +1,22 @@
+// RUN: %clang_cc1 -triple spir64 -aux-triple x86_64-unknown-linux-gnu -fsycl-is-device -verify -fsyntax-only %s
+
+template 
+__attribute__((sycl_kernel)) void kernel(Func kernelFunc) {
+  kernelFunc(); // expected-note {{called by 'kernel}}
+}
+
+void host_ok(void) {
+  __bf16 A;
+}
+
+int main()
+{  host_ok();
+  __bf16 var; // expected-note {{'var' defined here}}
+  kernel([=]() {
+(void)var; // expected-error {{'var' requires 16 bit size '__bf16' type support, but target 'spir64' does not support it}}
+int B = sizeof(__bf16);
+  });
+
+  return 0;
+}
+
Index: clang/lib/Sema/SemaType.cpp
===
--- clang/lib/Sema/SemaType.cpp
+++ clang/lib/Sema/SemaType.cpp
@@ -1518,9 +1518,10 @@
 break;
   case 

[PATCH] D141375: [SYCL][OpenMP] Fix compilation errors for unsupported __bf16 intrinsics

[Mike Rice via Phabricator via cfe-commits]

mikerice accepted this revision.
mikerice added a comment.

LGTM too.


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

https://reviews.llvm.org/D141375

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141375: [SYCL][OpenMP] Fix compilation errors for unsupported __bf16 intrinsics

[Elizabeth Andrews via Phabricator via cfe-commits]

eandrews added a comment.

In D141375#4041360 , @bader wrote:

> LGTM.
> I expect this to be a common issue for all single-source offloading 
> programming models (i.e. CUDA and HIP in addition to SYCL and OpenMP 
> offload). Probably we can generalize the code patterns used in this patch for 
> all of them.

Looks like CUDA added support for the type  - https://reviews.llvm.org/D136311, 
https://reviews.llvm.org/rG678d8946ba2ba790c4c52e96e2134ee136e30057.

>> In addition to that, there are other built-in data types not supported 
>> either by host or device, which are handled similar way. Right?

Yes. Code added here is similar to code added for other unsupported types like 
__float128


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

https://reviews.llvm.org/D141375

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141375: [SYCL][OpenMP] Fix compilation errors for unsupported __bf16 intrinsics

[Alexey Bader via Phabricator via cfe-commits]

bader accepted this revision.
bader added a comment.
This revision is now accepted and ready to land.

LGTM.

I expect this to be a common issue for all single-source offloading programming 
models (i.e. CUDA and HIP in addition to SYCL and OpenMP offload). Probably we 
can generalize the code patterns used in this patch for all of them.

In addition to that, there are other built-in data types not supported either 
by host or device, which are handled similar way. Right?


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

https://reviews.llvm.org/D141375

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141375: [SYCL][OpenMP] Fix compilation errors for unsupported __bf16 intrinsics

[Elizabeth Andrews via Phabricator via cfe-commits]

eandrews created this revision.
eandrews added reviewers: mikerice, jyu2, bader, jdoerfert, aaron.ballman.
Herald added subscribers: Naghasan, Anastasia, ebevhan, guansong, yaxunl.
Herald added a project: All.
eandrews requested review of this revision.
Herald added a subscriber: sstefan1.

This patch uses existing deferred diagnostics framework to emit error for 
unsupported type __bf16 in device code. Error is not emitted in host code.


https://reviews.llvm.org/D141375

Files:
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/ItaniumMangle.cpp
  clang/lib/Sema/Sema.cpp
  clang/lib/Sema/SemaType.cpp
  clang/test/SemaSYCL/bf16.cpp


Index: clang/test/SemaSYCL/bf16.cpp
===
--- /dev/null
+++ clang/test/SemaSYCL/bf16.cpp
@@ -0,0 +1,22 @@
+// RUN: %clang_cc1 -triple spir64 -aux-triple x86_64-unknown-linux-gnu 
-fsycl-is-device -verify -fsyntax-only %s
+
+template 
+__attribute__((sycl_kernel)) void kernel(Func kernelFunc) {
+  kernelFunc(); // expected-note {{called by 'kernel}}
+}
+
+void host_ok(void) {
+  __bf16 A;
+}
+
+int main()
+{  host_ok();
+  __bf16 var; // expected-note {{'var' defined here}}
+  kernel([=]() {
+(void)var; // expected-error {{'var' requires 16 bit size '__bf16' type 
support, but target 'spir64' does not support it}}
+int B = sizeof(__bf16);
+  });
+
+  return 0;
+}
+
Index: clang/lib/Sema/SemaType.cpp
===
--- clang/lib/Sema/SemaType.cpp
+++ clang/lib/Sema/SemaType.cpp
@@ -1518,9 +1518,10 @@
 break;
   case DeclSpec::TST_half:Result = Context.HalfTy; break;
   case DeclSpec::TST_BFloat16:
-if (!S.Context.getTargetInfo().hasBFloat16Type())
-  S.Diag(DS.getTypeSpecTypeLoc(), diag::err_type_unsupported)
-<< "__bf16";
+if (!S.Context.getTargetInfo().hasBFloat16Type() &&
+!(S.getLangOpts().OpenMP && S.getLangOpts().OpenMPIsDevice) &&
+!S.getLangOpts().SYCLIsDevice)
+  S.Diag(DS.getTypeSpecTypeLoc(), diag::err_type_unsupported) << "__bf16";
 Result = Context.BFloat16Ty;
 break;
   case DeclSpec::TST_float:   Result = Context.FloatTy; break;
Index: clang/lib/Sema/Sema.cpp
===
--- clang/lib/Sema/Sema.cpp
+++ clang/lib/Sema/Sema.cpp
@@ -1974,6 +1974,8 @@
 (Ty->isIbm128Type() && !Context.getTargetInfo().hasIbm128Type()) ||
 (Ty->isIntegerType() && Context.getTypeSize(Ty) == 128 &&
  !Context.getTargetInfo().hasInt128Type()) ||
+(Ty->isBFloat16Type() && !Context.getTargetInfo().hasBFloat16Type() &&
+ !LangOpts.CUDAIsDevice) ||
 LongDoubleMismatched) {
   PartialDiagnostic PD = PDiag(diag::err_target_unsupported_type);
   if (D)
Index: clang/lib/AST/ItaniumMangle.cpp
===
--- clang/lib/AST/ItaniumMangle.cpp
+++ clang/lib/AST/ItaniumMangle.cpp
@@ -3050,7 +3050,11 @@
 break;
   }
   case BuiltinType::BFloat16: {
-const TargetInfo *TI = ().getTargetInfo();
+const TargetInfo *TI = ((getASTContext().getLangOpts().OpenMP &&
+ getASTContext().getLangOpts().OpenMPIsDevice) ||
+getASTContext().getLangOpts().SYCLIsDevice)
+   ? getASTContext().getAuxTargetInfo()
+   : ().getTargetInfo();
 Out << TI->getBFloat16Mangling();
 break;
   }
Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -2141,6 +2141,11 @@
   if (Target->hasBFloat16Type()) {
 Width = Target->getBFloat16Width();
 Align = Target->getBFloat16Align();
+  } else if ((getLangOpts().SYCLIsDevice ||
+  (getLangOpts().OpenMP && getLangOpts().OpenMPIsDevice)) &&
+ AuxTarget->hasBFloat16Type()) {
+Width = AuxTarget->getBFloat16Width();
+Align = AuxTarget->getBFloat16Align();
   }
   break;
 case BuiltinType::Float16:


Index: clang/test/SemaSYCL/bf16.cpp
===
--- /dev/null
+++ clang/test/SemaSYCL/bf16.cpp
@@ -0,0 +1,22 @@
+// RUN: %clang_cc1 -triple spir64 -aux-triple x86_64-unknown-linux-gnu -fsycl-is-device -verify -fsyntax-only %s
+
+template 
+__attribute__((sycl_kernel)) void kernel(Func kernelFunc) {
+  kernelFunc(); // expected-note {{called by 'kernel}}
+}
+
+void host_ok(void) {
+  __bf16 A;
+}
+
+int main()
+{  host_ok();
+  __bf16 var; // expected-note {{'var' defined here}}
+  kernel([=]() {
+(void)var; // expected-error {{'var' requires 16 bit size '__bf16' type support, but target 'spir64' does not support it}}
+int B = sizeof(__bf16);
+  });
+
+  return 0;
+}
+
Index: clang/lib/Sema/SemaType.cpp
===
---