[PATCH] D32896: [OpenCL] Make CLK_NULL_RESERVE_ID invalid reserve id.

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.


================
Comment at: lib/Headers/opencl-c.h:16020
+// 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);
----------------
echuraev wrote:
> yaxunl wrote:
> > Anastasia wrote:
> > > yaxunl wrote:
> > > > Anastasia wrote:
> > > > > 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`.
> > > > `__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?
> > > 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.
> > 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.
> 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. 
It is more logical to me that `CLK_NULL_RESERVE_ID` is 0, but spec doesn't define it of course.


https://reviews.llvm.org/D32896