[OpenCL] Make CLK_NULL_RESERVE_ID invalid reserve id.

[echuraev]

Summary

Make CLK_NULL_RESERVE_ID invalid reserve id.

Rename PIPE_RESERVE_ID_VALID_BIT to avoid user name space pollution.

Current implementation reserve_id_t type assumes that it's a pointer
type whose most significant bit is set to one and the rest of the bits
keep the id value.

This patch increase reserve id size by one bit on 32-bit platforms and
by 33 bits on 64-bit platforms.

[Anastasia]

lib/Headers/opencl-c.h

16020

Looks good from my side.

@yaxunl , since you originally committed this. Could you please verify that changing from SIZE_MAX to 0 would be fine.

Btw, we have a similar definition for CLK_NULL_EVENT.

[yaxunl]

lib/Headers/opencl-c.h

16020

__PIPE_RESERVE_ID_VALID_BIT is implementation detail and not part of the spec. I would suggest to remove it from this header file.

The spec only requires CLK_NULL_RESERVE_ID to be defined but does not define its value. Naturally a valid id starts from 0 and increases. I don't see significant advantage to change CLK_NULL_RESERVE_ID from __SIZE_MAX to 0.

Is there any reason that this change is needed?

[Anastasia]

lib/Headers/opencl-c.h

16020

I don't see issues to commit things outside of spec as soon as they prefixed properly with "__". But I agree it would be nice to see if it's any useful and what the motivation is for having different implementation.

[yaxunl]

lib/Headers/opencl-c.h

16020

For __PIPE_RESERVE_ID_VALID_BIT, it assumes that the implementation uses one specific bit of a reserve id to indicate that the reserve id is valid. Not all implementations assume that. Actually I am curious why that is needed too.

[echuraev]

lib/Headers/opencl-c.h

16020

About CLK_NULL_RESERVE_ID: we check that reserve id is valid if significant bit equal to one. CLK_NULL_RESERVE_ID refers to an invalid reservation, so if CLK_NULL_RESERVE_ID equal to 0, we can be sure that significant bit doesn't equal to 1 and it is invalid reserve id. Also it is more obviously if CLK_NULL_RESERVE_ID is equal to 0.

What about __PIPE_RESERVE_ID_VALID_BIT: As I understand previous implementation also assumes that one specific bit was of a reverse id was used to indicate that the reserve id is valid. So, we just increased reserve id size by one bit on 32-bit platforms and by 33 bits on 64-bit platforms.

[Anastasia]

lib/Headers/opencl-c.h

16020

It is more logical to me that CLK_NULL_RESERVE_ID is 0, but spec doesn't define it of course.

[yaxunl]

lib/Headers/opencl-c.h

16020

In our implementation, valid reserve id starts at 0 and increasing linearly until __SIZE_MAX-1. This change will break our implementation.

However, we can modify our implementation to adopt this change since it brings about benefits overall.

[bader]

lib/Headers/opencl-c.h

16020

Ideally it would be great to have unified implementation, but we can define device specific value for CLK_NULL_RESERVE_ID by using ifdef directive.

[yaxunl]

lib/Headers/opencl-c.h

16020

How about

__attribute__((const)) size_t __clk_null_reserve_id();
#define CLK_NULL_RESERVE_ID __clk_null_reserve_id()

I think the spec does not require it to be compile time constant. Then each library can implement its own __clk_null_reserve_id() whereas the IR is target independent.

[yaxunl]

lib/Headers/opencl-c.h

16020

Or we only do this for SPIR and define it as target specific value for other targets.

[bader]

lib/Headers/opencl-c.h

16020

Defining CLK_NULL_RESERVE_ID as a function call should also work, but IMHO compile time constant is preferable option.
I don't think making it compile time constant for SPIR only makes sense to me - in this case we can use constant for all targets.

How about following approach: use 0 by default and allow other targets re-define CLK_NULL_RESERVE_ID value.

#ifndef CLK_NULL_RESERVE_ID
  #define CLK_NULL_RESERVE_ID 0
#endif // CLK_NULL_RESERVE_ID

If CLK_NULL_RESERVE_ID defined via -D command line option or included before OpenCL C header file (via -include option), the defined value will be used, otherwise 0.

Will it work for you?

[yaxunl]

lib/Headers/opencl-c.h

16020

No. That makes us unable to consume SPIR since CLK_NULL_RESERVE_ID is hardcoded as 0 when the source code is translated to SPIR whereas our target expects ~0U.

To get a portable IR we need to represent CLK_NULL_RESERVE_ID as a function which can be lowered to a constant by a specific target.

[bader]

lib/Headers/opencl-c.h

16020

Do you refer to SPIR-V or SPIR 2.0?

BTW, you also mentioned that it's possible to up modify your implementation to align it with proposed version.

However, we can modify our implementation to adopt this change since it brings about benefits overall.

Is it still an option?

[yaxunl]

lib/Headers/opencl-c.h

16020

I was talking about SPIR in a general sense. What I said applies to either SPIR or SPIR-V, as long as we want a portable representation. Currently Clang does not support SPIR-V, but I guess the header file will be used for SPIR-V in the future in the similar way as it is used for SPIR.

Yes, that is still an option. In that case I want a uniform compile-time constant definition for all targets.

If we choose to allow each target having its own constant value, then for SPIR or SPIR-V it needs to be defined as a function.

[bader]

lib/Headers/opencl-c.h

16020

What I said applies to either SPIR or SPIR-V, as long as we want a portable representation. ...
If we choose to allow each target having its own constant value, then for SPIR or SPIR-V it needs to be defined as a function.

SPIR-V (as well as OpenCL) doesn't define CLK_NULL_RESERVE_ID, so it's not portable. Replacing it with function call puts additional implicit requirements for the frameworks consuming SPIR-V to provide __clk_null_reserve_id function implementation.

Yes, that is still an option. In that case I want a uniform compile-time constant definition for all targets.

So far, I'm in favor of this option. I think we need to bring this question to the Khronos and either:

1. define uniform compile-time constant
2. make the OpenCL value implementation defined and introduce SPIR-V representation for CLK_NULL_RESERVE_ID

[yaxunl]

lib/Headers/opencl-c.h

16020

Agree.

[b-sumner]

lib/Headers/opencl-c.h

16020

I wish I understood why we have both CLK_NULL_RESERVE_ID and the function is_valid_reserve_id(). We don't need both, and the predicate makes the code clearer. Also, that macro implies that == and != must be defined for reserve_id_t which is again undesirable.

I'd say any CLK_* value is automatically implementation defined if its value is not defined in the spec, but wouldn't object to clarifying with the working group.

[bader]

lib/Headers/opencl-c.h

16020

Sorry, I didn't update this code review thread, but I emailed to the working group regarding whether this macro should be implementation defined or set by the spec, since we need some portable binary representation for SPIR-V format. This thread didn't received any attention, but I think I found the right SPIR-V representation for this particular case.

@b-sumner brought another good question. It would be great if could resolve this ambiguity by removing CLK_NULL_RESERVE_ID macro.

lib/Headers/opencl-c.h

 double __ovld __conv work_group_scan_inclusive_min(double x);
 double __ovld __conv work_group_scan_inclusive_max(double x);
 #endif //cl_khr_fp64
 
 #endif //__OPENCL_C_VERSION__ >= CL_VERSION_2_0
 
 // OpenCL v2.0 s6.13.16 - Pipe Functions
 #if __OPENCL_C_VERSION__ >= CL_VERSION_2_0
-#define PIPE_RESERVE_ID_VALID_BIT (1U << 30)
-#define CLK_NULL_RESERVE_ID (__builtin_astype(((void*)(__SIZE_MAX__)), reserve_id_t))
+// The most significant bit of valid reserve_id must be set to one.
+#define __PIPE_RESERVE_ID_VALID_BIT ((size_t)1 << (sizeof(size_t) * 8 - 1))
+// The macro CLK_NULL_RESERVE_ID refers to an invalid reservation ID.
+#define CLK_NULL_RESERVE_ID (__builtin_astype((void *)0, reserve_id_t))
 bool __ovld is_valid_reserve_id(reserve_id_t reserve_id);
 #endif //__OPENCL_C_VERSION__ >= CL_VERSION_2_0
 
 
 // OpenCL v2.0 s6.13.17 - Enqueue Kernels
 #if __OPENCL_C_VERSION__ >= CL_VERSION_2_0
 
 #define CL_COMPLETE                                 0x0