[PATCH] D89372: [OpenCL] Remove unused extensions

[Jan Vesely via Phabricator via cfe-commits]

jvesely added a comment.

In D89372#2332997 <https://reviews.llvm.org/D89372#2332997>, @Anastasia wrote:

> In D89372#2332853 <https://reviews.llvm.org/D89372#2332853>, @jvesely wrote:
>
>> `cl_khr_byte_addressable_stores` changes language semantics. without it, pointer dereferences of types smaller than 32 bits are illegal.
>
> Ok, does it mean that we are missing to diagnose this in the frontend? Btw I do acknowledge that what you say makes sense but I don't think the documentation support that:
> https://www.khronos.org/registry/OpenCL/sdk/2.2/docs/man/html/cl_khr_byte_addressable_store.html
>
> Am I just looking in the wrong place?

Since it's only an extension in ocl 1.0 that spec is a better place to look:

  9.9 Byte Addressable Stores 
  Section 6.8.m describes restrictions on built-in types char, uchar, char2, uchar2, short,
  and half. The OpenCL extension cl_khr_byte_addressable_store removes these
  restrictions.  An application that wants to be able to write to elements of a pointer (or struct) that
  are of type char, uchar, char2, uchar2, short, ushort and half will need to include
  the #pragma OPENCL EXTENSION cl_khr_byte_addressable_store :
  enable directive before any code that performs writes that may not be supported as per section
  6.8.m.
  
  6.8 Restrictions
  ...
  m. Built-in types that are less than 32-bits in size i.e. char, uchar, char2, uchar2,
  short, ushort, and half have the following restriction:
  Writes to a pointer (or arrays) of type char, uchar, char2, uchar2, short,
  ushort, and half or to elements of a struct that are of type char, uchar,
  char2, uchar2, short and ushort are not supported. Refer to section 9.9
  for additional information.



>> Even if all clang targets support this the macro should still be defined for backward compatibility.
>
> Ok, are you saying that all targets currently support this and we sould always define it? In this case I would be more happy if we move them into the internal header that we add implicitly anyway...

r600/r700/EG/NI targets have a bit wonky support byte-addressable stores (basically using 32bit atomic MSKOR), but I don't expect those targets to survive for long, and for now, they advertise support.
some out of tree backends might also benefit from the extension (like legup -- verilog backend), but I'm not sure if out of tree targets are worth considering, or if they indeed make use of this extension.
on a high-level, I could see a restriction to 32bit data paths be useful for FPGA targets.

moving the define it to header breaks OCL1.0 programs. Based on the specs above. those programs are required to include the following:

  #ifdef cl_khr_byte_addressable_store
  #pragma OPENCL EXTENSION cl_khr_byte_addressable_store : enable
  #endif

Before they can dereference pointers to char/char2/short/... types (including array indexing and struct members).
so the `pragma` part needs to work, and the language level checks need to work if the extension is not enabled.

The same arguments also apply to `cles_khr_in64`. It's illegal to use int64 types in embedded profile unless you first enable the extensions. Rather than removing it, `cles_khr_2d_image_array_writes` should be added to the extension list.

clang can always decide that OCL1.0 programs (even when compiled in cl-1.x mode) and embedded profile is unsupported.
I have no clue if there are many OCL programs that rely on those modes out there.
However, removing support for them is a significantly bigger change than cleaning up a few language-irrelevant extensions (actually, I'm not sure if clang ever supported OCL embedded mode).

>> it would be useful if the commit message or the description of this revision included a justification for each removed extension why it doesn't impact language semantics with spec references.
>
> Yes, this is a good suggestion in principle and we should try our best. However I am not sure it is feasible for all of those, in particular this documentation doesn't contain anything:
> https://man.opencl.org/cl_khr_context_abort.html
>
> Are you suggesting to leave this out? However I don't see any evidence that this require either macro or pragma. I feel this is in rather incomplete state. So I don't feel we can accomodate for all of these.

"incomplete specification" is imo a good reason to drop support for an extension. My argument is that explanation of legacy extensions should rely on the spec that introduced them, rather than the current 2.x/3.x which does an arguably poor job on backward compatibility.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89372