jvesely added a comment.

In D77923#1981398 <https://reviews.llvm.org/D77923#1981398>, @arsenm wrote:

> In D77923#1979286 <https://reviews.llvm.org/D77923#1979286>, @jvesely wrote:
>
> > > In D77923#1976497 <https://reviews.llvm.org/D77923#1976497>, @jvesely 
> > > wrote:
> > > 
> > >> OPENCL_VERSION is something that should be really set by the opencl 
> > >> driver rather than the compiler.
> > >>  coarse-grained SVM can be done without FEATURE_FLAT_ADDRESS_SPACE, so 
> > >> those gpus can get over 1.2, while the compiler can be used in an 
> > >> implementation that doesn't go above 1.2 even for gfx700+
> > > 
> > > 
> > > I don't know why this would imply we can't just have a static 
> > > `__OPENCL_VERSION__` for each device in Clang? I don't see anything in 
> > > the standard that says you are not allowed to have (going with your 
> > > example) `__OPENCL_VERSION__=200` and `__OPENCL_C_VERSION=120` at the 
> > > same time.
> > > 
> > > It seems like we should be able to just have `__OPENCL_VERSION__` defined 
> > > in Clang, and let the runtime control setting `__OPENCL_C_VERSION__` via 
> > > `-cl-std=`. If the current patch sets `__OPENCL_VERSION__` differently 
> > > than the runtime does then it seems like we should make it match, but I'm 
> > > not sure how best to confirm that.
> >
> > The `__OPENCL_VERSION__` macro in the kernel needs to match the version 
> > returned by `clGetDeviceInfo` (both refer to OpenCL version supported by 
> > the device),
> >  which in turn must be <= version returned by `clGetPlatformInfo`.
> >  This part makes it unsuitable for hardcoding in the compiler.
> >
> > `__OPENCL_VERSION__` should indicate device capabilities rather than 
> > language features. specs talk about available resources.
> >  For example, `CL_DEVICE_IMAGE2D_MAX_HEIGHT` is required to be >= 8192 for 
> > OpenCL 1.2 while it's required to be >=16384 for OpenCL 2.1.
>
>
> But why would the driver not report the device values which we do know in the 
> compiler? A discrepancy between these sounds like a driver bug.


because it needs to be capped by the platform opencl version. devices 
supporting opencl x.y need to support opencl <x.y as well, so capping it at 
lower version is ok.

>> if there's to be a default value hardcoded in the compiler it'd be nice if 
>> platform drivers had an easy way to override (lower) it.
> 
> I found a hacky way to do this. You can -include a header, which happens 
> after the predefines, with undef and redef of the predefined value. This does 
> trigger -Wreserved-id-macro (which I only found through -Weverything), but 
> you should be able to pragma push/pop the warning off inside the special 
> header.

does passing `-U__OPENCL_VERSION__ -D __OPENCL_VERSION__=x.y` on the commadline 
work as well? the header trick was my fallback path.

> For now I could assume the platform is 2.0 based on -amdhsa, and not for 
> -mesa3d to work around clover not currently supporting CL2.0.




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

https://reviews.llvm.org/D77923



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

Reply via email to