[PATCH] D77910: AMDGPU: Define cl_khr_gl_sharing as a supported extension

[Brian Sumner via Phabricator via cfe-commits]

b-sumner added a comment.

In D77910#2015225 , @arsenm wrote:

> In D77910#1989163 , @b-sumner wrote:
>
> > In D77910#1988807 , @arsenm wrote:
> >
> > > In D77910#1981828 , @b-sumner 
> > > wrote:
> > >
> > > > In D77910#1981429 , @arsenm 
> > > > wrote:
> > > >
> > > > > In D77910#1976171 , 
> > > > > @b-sumner wrote:
> > > > >
> > > > > > I don't think we can guarantee this is or will be supported on all 
> > > > > > devices.  The language runtime makes this decision.
> > > > >
> > > > >
> > > > > We don't need to worry about theoretical devices. We should know the 
> > > > > properties of the driver from -amdhsa, -amdpal, -mesa3d
> > > >
> > > >
> > > > It takes more than support in the ISA for some features.  The OpenCL 
> > > > driver may not want to support a given optional feature, e.g. images.  
> > > > I'm not opposed to defaults, but if the driver chooses to not support 
> > > > images, it needs to be able to prevent `__IMAGE_SUPPORT__` from being 
> > > > defined.  Conformance will fail if the runtime and compiler are not 
> > > > consistent.
> > >
> > >
> > > The driver details should be captured by the the triple. If some weird 
> > > driver decided to do something different, we would need to add a new 
> > > triple for it. We don't have such a driver, so I don't see why worry 
> > > about it. It's possible to work around with undef and redef in an 
> > > implicitly included header. We need to fix properties of the driver based 
> > > on the target to have perfectly matching offline compilation
> >
> >
> > I don't see anywhere in the triple talking about driver specific details, 
> > unless you would use the environment?  That seems like overkill to me.  But 
> > again, I'm not opposed to defaults, and as long as the driver can override 
> > them, this should be OK.
>
>
> The OS is the driver. It doesn't need to specifically encode these details; 
> the OS should encode properties of the driver environment. Anything using 
> -amdhsa should be reporting image support


But that's not how things work now or are likely to work in the future.  The 
language runtime is what decides if it is going to report the availability of 
image support.  For offline compilation, I suppose it is OK to assume images 
are supported, but for online compilation, the language runtime needs to be 
able to reflect its own decision.


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

https://reviews.llvm.org/D77910



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

[PATCH] D77910: AMDGPU: Define cl_khr_gl_sharing as a supported extension

[Matt Arsenault via Phabricator via cfe-commits]

arsenm added a comment.

In D77910#1989163 , @b-sumner wrote:

> In D77910#1988807 , @arsenm wrote:
>
> > In D77910#1981828 , @b-sumner 
> > wrote:
> >
> > > In D77910#1981429 , @arsenm 
> > > wrote:
> > >
> > > > In D77910#1976171 , @b-sumner 
> > > > wrote:
> > > >
> > > > > I don't think we can guarantee this is or will be supported on all 
> > > > > devices.  The language runtime makes this decision.
> > > >
> > > >
> > > > We don't need to worry about theoretical devices. We should know the 
> > > > properties of the driver from -amdhsa, -amdpal, -mesa3d
> > >
> > >
> > > It takes more than support in the ISA for some features.  The OpenCL 
> > > driver may not want to support a given optional feature, e.g. images.  
> > > I'm not opposed to defaults, but if the driver chooses to not support 
> > > images, it needs to be able to prevent `__IMAGE_SUPPORT__` from being 
> > > defined.  Conformance will fail if the runtime and compiler are not 
> > > consistent.
> >
> >
> > The driver details should be captured by the the triple. If some weird 
> > driver decided to do something different, we would need to add a new triple 
> > for it. We don't have such a driver, so I don't see why worry about it. 
> > It's possible to work around with undef and redef in an implicitly included 
> > header. We need to fix properties of the driver based on the target to have 
> > perfectly matching offline compilation
>
>
> I don't see anywhere in the triple talking about driver specific details, 
> unless you would use the environment?  That seems like overkill to me.  But 
> again, I'm not opposed to defaults, and as long as the driver can override 
> them, this should be OK.


The OS is the driver. It doesn't need to specifically encode these details; the 
OS should encode properties of the driver environment. Anything using -amdhsa 
should be reporting image support


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

https://reviews.llvm.org/D77910



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

[PATCH] D77910: AMDGPU: Define cl_khr_gl_sharing as a supported extension

[Brian Sumner via Phabricator via cfe-commits]

b-sumner added a comment.

In D77910#1988807 , @arsenm wrote:

> In D77910#1981828 , @b-sumner wrote:
>
> > In D77910#1981429 , @arsenm wrote:
> >
> > > In D77910#1976171 , @b-sumner 
> > > wrote:
> > >
> > > > I don't think we can guarantee this is or will be supported on all 
> > > > devices.  The language runtime makes this decision.
> > >
> > >
> > > We don't need to worry about theoretical devices. We should know the 
> > > properties of the driver from -amdhsa, -amdpal, -mesa3d
> >
> >
> > It takes more than support in the ISA for some features.  The OpenCL driver 
> > may not want to support a given optional feature, e.g. images.  I'm not 
> > opposed to defaults, but if the driver chooses to not support images, it 
> > needs to be able to prevent `__IMAGE_SUPPORT__` from being defined.  
> > Conformance will fail if the runtime and compiler are not consistent.
>
>
> The driver details should be captured by the the triple. If some weird driver 
> decided to do something different, we would need to add a new triple for it. 
> We don't have such a driver, so I don't see why worry about it. It's possible 
> to work around with undef and redef in an implicitly included header. We need 
> to fix properties of the driver based on the target to have perfectly 
> matching offline compilation


I don't see anywhere in the triple talking about driver specific details, 
unless you would use the environment?  That seems like overkill to me.  But 
again, I'm not opposed to defaults, and as long as the driver can override 
them, this should be OK.


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

https://reviews.llvm.org/D77910



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

[PATCH] D77910: AMDGPU: Define cl_khr_gl_sharing as a supported extension

[Matt Arsenault via Phabricator via cfe-commits]

arsenm added a comment.

In D77910#1981828 , @b-sumner wrote:

> In D77910#1981429 , @arsenm wrote:
>
> > In D77910#1976171 , @b-sumner 
> > wrote:
> >
> > > I don't think we can guarantee this is or will be supported on all 
> > > devices.  The language runtime makes this decision.
> >
> >
> > We don't need to worry about theoretical devices. We should know the 
> > properties of the driver from -amdhsa, -amdpal, -mesa3d
>
>
> It takes more than support in the ISA for some features.  The OpenCL driver 
> may not want to support a given optional feature, e.g. images.  I'm not 
> opposed to defaults, but if the driver chooses to not support images, it 
> needs to be able to prevent `__IMAGE_SUPPORT__` from being defined.  
> Conformance will fail if the runtime and compiler are not consistent.


The driver details should be captured by the the triple. If some weird driver 
decided to do something different, we would need to add a new triple for it. We 
don't have such a driver, so I don't see why worry about it. It's possible to 
work around with undef and redef in an implicitly included header. We need to 
fix properties of the driver based on the target to have perfectly matching 
offline compilation


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

https://reviews.llvm.org/D77910



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

[PATCH] D77910: AMDGPU: Define cl_khr_gl_sharing as a supported extension

[Brian Sumner via Phabricator via cfe-commits]

b-sumner added a comment.

In D77910#1981429 , @arsenm wrote:

> In D77910#1976171 , @b-sumner wrote:
>
> > I don't think we can guarantee this is or will be supported on all devices. 
> >  The language runtime makes this decision.
>
>
> We don't need to worry about theoretical devices. We should know the 
> properties of the driver from -amdhsa, -amdpal, -mesa3d


It takes more than support in the ISA for some features.  The OpenCL driver may 
not want to support a given optional feature, e.g. images.  I'm not opposed to 
defaults, but if the driver chooses to not support images, it needs to be able 
to prevent `__IMAGE_SUPPORT__` from being defined.  Conformance will fail if 
the runtime and compiler are not consistent.


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

https://reviews.llvm.org/D77910



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

[PATCH] D77910: AMDGPU: Define cl_khr_gl_sharing as a supported extension

[Matt Arsenault via Phabricator via cfe-commits]

arsenm updated this revision to Diff 257470.
arsenm added a comment.

Check triple OS


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

https://reviews.llvm.org/D77910

Files:
  clang/lib/Basic/Targets/AMDGPU.h
  clang/test/Misc/amdgcn.languageOptsOpenCL.cl


Index: clang/test/Misc/amdgcn.languageOptsOpenCL.cl
===
--- clang/test/Misc/amdgcn.languageOptsOpenCL.cl
+++ clang/test/Misc/amdgcn.languageOptsOpenCL.cl
@@ -8,6 +8,20 @@
 // RUN: %clang_cc1 -x cl -cl-std=CL1.2 %s -verify -triple 
amdgcn-unknown-unknown -Wpedantic-core-features -DTEST_CORE_FEATURES
 // RUN: %clang_cc1 -x cl -cl-std=CL2.0 %s -verify -triple 
amdgcn-unknown-unknown -Wpedantic-core-features -DTEST_CORE_FEATURES
 
+// Test extensions supported by amdhsa and amdpal
+// RUN: %clang_cc1 -x cl -cl-std=CL1.1 %s -verify -triple 
amdgcn-unknown-amdhsa -Wpedantic-core-features -DTEST_CORE_FEATURES -DHSAPAL
+// RUN: %clang_cc1 -x cl -cl-std=CL1.2 %s -verify -triple 
amdgcn-unknown-amdhsa -Wpedantic-core-features -DTEST_CORE_FEATURES -DHSAPAL
+// RUN: %clang_cc1 -x cl -cl-std=CL2.0 %s -verify -triple 
amdgcn-unknown-amdhsa -Wpedantic-core-features -DTEST_CORE_FEATURES -DHSAPAL
+// RUN: %clang_cc1 -x cl -cl-std=CL1.1 %s -verify -triple 
amdgcn-unknown-amdpal -Wpedantic-core-features -DTEST_CORE_FEATURES -DHSAPAL
+// RUN: %clang_cc1 -x cl -cl-std=CL1.2 %s -verify -triple 
amdgcn-unknown-amdpal -Wpedantic-core-features -DTEST_CORE_FEATURES -DHSAPAL
+// RUN: %clang_cc1 -x cl -cl-std=CL2.0 %s -verify -triple 
amdgcn-unknown-amdpal -Wpedantic-core-features -DTEST_CORE_FEATURES -DHSAPAL
+
+// Test extensions supported by mesa3d/clover
+// RUN: %clang_cc1 -x cl -cl-std=CL1.1 %s -verify -triple 
amdgcn-unknown-mesa3d -Wpedantic-core-features -DTEST_CORE_FEATURES
+// RUN: %clang_cc1 -x cl -cl-std=CL1.2 %s -verify -triple 
amdgcn-unknown-mesa3d -Wpedantic-core-features -DTEST_CORE_FEATURES
+// RUN: %clang_cc1 -x cl -cl-std=CL2.0 %s -verify -triple 
amdgcn-unknown-mesa3d -Wpedantic-core-features -DTEST_CORE_FEATURES
+
+
 // Extensions in all versions
 #ifndef cl_clang_storage_class_specifiers
 #error "Missing cl_clang_storage_class_specifiers define"
@@ -29,11 +43,20 @@
 #endif
 #pragma OPENCL EXTENSION cl_khr_int64_extended_atomics: enable
 
+#ifdef HSAPAL
+#ifndef cl_khr_gl_sharing
+#error "Incorrect cl_khr_gl_sharing define"
+#endif
+#pragma OPENCL EXTENSION cl_khr_gl_sharing: enable
+
+#else
+
 #ifdef cl_khr_gl_sharing
 #error "Incorrect cl_khr_gl_sharing define"
 #endif
 #pragma OPENCL EXTENSION cl_khr_gl_sharing: enable
 // expected-warning@-1{{unsupported OpenCL extension 'cl_khr_gl_sharing' - 
ignoring}}
+#endif
 
 #ifndef cl_khr_icd
 #error "Missing cl_khr_icd define"
Index: clang/lib/Basic/Targets/AMDGPU.h
===
--- clang/lib/Basic/Targets/AMDGPU.h
+++ clang/lib/Basic/Targets/AMDGPU.h
@@ -267,6 +267,11 @@
   Opts.support("cl_khr_mipmap_image_writes");
   Opts.support("cl_khr_subgroups");
   Opts.support("cl_khr_3d_image_writes");
+
+  if (getTriple().getOS() == llvm::Triple::AMDHSA ||
+  getTriple().getOS() == llvm::Triple::AMDPAL)
+Opts.support("cl_khr_gl_sharing");
+
   Opts.support("cl_amd_media_ops");
   Opts.support("cl_amd_media_ops2");
 }


Index: clang/test/Misc/amdgcn.languageOptsOpenCL.cl
===
--- clang/test/Misc/amdgcn.languageOptsOpenCL.cl
+++ clang/test/Misc/amdgcn.languageOptsOpenCL.cl
@@ -8,6 +8,20 @@
 // RUN: %clang_cc1 -x cl -cl-std=CL1.2 %s -verify -triple amdgcn-unknown-unknown -Wpedantic-core-features -DTEST_CORE_FEATURES
 // RUN: %clang_cc1 -x cl -cl-std=CL2.0 %s -verify -triple amdgcn-unknown-unknown -Wpedantic-core-features -DTEST_CORE_FEATURES
 
+// Test extensions supported by amdhsa and amdpal
+// RUN: %clang_cc1 -x cl -cl-std=CL1.1 %s -verify -triple amdgcn-unknown-amdhsa -Wpedantic-core-features -DTEST_CORE_FEATURES -DHSAPAL
+// RUN: %clang_cc1 -x cl -cl-std=CL1.2 %s -verify -triple amdgcn-unknown-amdhsa -Wpedantic-core-features -DTEST_CORE_FEATURES -DHSAPAL
+// RUN: %clang_cc1 -x cl -cl-std=CL2.0 %s -verify -triple amdgcn-unknown-amdhsa -Wpedantic-core-features -DTEST_CORE_FEATURES -DHSAPAL
+// RUN: %clang_cc1 -x cl -cl-std=CL1.1 %s -verify -triple amdgcn-unknown-amdpal -Wpedantic-core-features -DTEST_CORE_FEATURES -DHSAPAL
+// RUN: %clang_cc1 -x cl -cl-std=CL1.2 %s -verify -triple amdgcn-unknown-amdpal -Wpedantic-core-features -DTEST_CORE_FEATURES -DHSAPAL
+// RUN: %clang_cc1 -x cl -cl-std=CL2.0 %s -verify -triple amdgcn-unknown-amdpal -Wpedantic-core-features -DTEST_CORE_FEATURES -DHSAPAL
+
+// Test extensions supported by mesa3d/clover
+// RUN: %clang_cc1 -x cl -cl-std=CL1.1 %s -verify -triple amdgcn-unknown-mesa3d -Wpedantic-core-features -DTEST_CORE_FEATURES
+// RUN: %clang_cc1 -x cl -cl-std=CL1.2 %s -verify -triple amdgcn-unknown-mesa3d -Wpedantic-core-features 

[PATCH] D77910: AMDGPU: Define cl_khr_gl_sharing as a supported extension

[Matt Arsenault via Phabricator via cfe-commits]

arsenm added a comment.

In D77910#1976171 , @b-sumner wrote:

> I don't think we can guarantee this is or will be supported on all devices.  
> The language runtime makes this decision.


We don't need to worry about theoretical devices. We should know the properties 
of the driver from -amdhsa, -amdpal, -mesa3d


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

https://reviews.llvm.org/D77910



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

[PATCH] D77910: AMDGPU: Define cl_khr_gl_sharing as a supported extension

[Brian Sumner via Phabricator via cfe-commits]

b-sumner added a comment.

I don't think we can guarantee this is or will be supported on all devices.  
The language runtime makes this decision.


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

https://reviews.llvm.org/D77910



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

[PATCH] D77910: AMDGPU: Define cl_khr_gl_sharing as a supported extension

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

I am not so sure whether compiler should always report this.

I thought it depends on platform and should let runtime to add it when it is 
really supported.

But on the other hand, we could report it by default if it is the most case, 
and let runtime to disable it if it is not supported.

So I will defer this to Brian.


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

https://reviews.llvm.org/D77910



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

[PATCH] D77910: AMDGPU: Define cl_khr_gl_sharing as a supported extension

[Matt Arsenault via Phabricator via cfe-commits]

arsenm created this revision.
arsenm added reviewers: yaxunl, Anastasia.
Herald added subscribers: danielkiss, kerbowa, kristof.beyls, t-tye, tpr, 
dstuttard, nhaehnle, wdng, jvesely, kzhuravl.

This doesn't seem to be that useful, since it doesn't change any
device side functions. Report it since it seems a macro should be
reported for every extension.

  

ROCm already reports this. Clover doesn't report it, but since this
isn't useful for device code, it probably isn't harmful to always
define the macro.


https://reviews.llvm.org/D77910

Files:
  clang/lib/Basic/Targets/AMDGPU.h
  clang/test/Misc/amdgcn.languageOptsOpenCL.cl


Index: clang/test/Misc/amdgcn.languageOptsOpenCL.cl
===
--- clang/test/Misc/amdgcn.languageOptsOpenCL.cl
+++ clang/test/Misc/amdgcn.languageOptsOpenCL.cl
@@ -29,11 +29,10 @@
 #endif
 #pragma OPENCL EXTENSION cl_khr_int64_extended_atomics: enable
 
-#ifdef cl_khr_gl_sharing
+#ifndef cl_khr_gl_sharing
 #error "Incorrect cl_khr_gl_sharing define"
 #endif
 #pragma OPENCL EXTENSION cl_khr_gl_sharing: enable
-// expected-warning@-1{{unsupported OpenCL extension 'cl_khr_gl_sharing' - 
ignoring}}
 
 #ifndef cl_khr_icd
 #error "Missing cl_khr_icd define"
Index: clang/lib/Basic/Targets/AMDGPU.h
===
--- clang/lib/Basic/Targets/AMDGPU.h
+++ clang/lib/Basic/Targets/AMDGPU.h
@@ -267,6 +267,7 @@
   Opts.support("cl_khr_mipmap_image_writes");
   Opts.support("cl_khr_subgroups");
   Opts.support("cl_khr_3d_image_writes");
+  Opts.support("cl_khr_gl_sharing");
   Opts.support("cl_amd_media_ops");
   Opts.support("cl_amd_media_ops2");
 }


Index: clang/test/Misc/amdgcn.languageOptsOpenCL.cl
===
--- clang/test/Misc/amdgcn.languageOptsOpenCL.cl
+++ clang/test/Misc/amdgcn.languageOptsOpenCL.cl
@@ -29,11 +29,10 @@
 #endif
 #pragma OPENCL EXTENSION cl_khr_int64_extended_atomics: enable
 
-#ifdef cl_khr_gl_sharing
+#ifndef cl_khr_gl_sharing
 #error "Incorrect cl_khr_gl_sharing define"
 #endif
 #pragma OPENCL EXTENSION cl_khr_gl_sharing: enable
-// expected-warning@-1{{unsupported OpenCL extension 'cl_khr_gl_sharing' - ignoring}}
 
 #ifndef cl_khr_icd
 #error "Missing cl_khr_icd define"
Index: clang/lib/Basic/Targets/AMDGPU.h
===
--- clang/lib/Basic/Targets/AMDGPU.h
+++ clang/lib/Basic/Targets/AMDGPU.h
@@ -267,6 +267,7 @@
   Opts.support("cl_khr_mipmap_image_writes");
   Opts.support("cl_khr_subgroups");
   Opts.support("cl_khr_3d_image_writes");
+  Opts.support("cl_khr_gl_sharing");
   Opts.support("cl_amd_media_ops");
   Opts.support("cl_amd_media_ops2");
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits