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

[Alexey Bader via Phabricator via cfe-commits]

bader 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);
----------------
yaxunl wrote:
> Anastasia wrote:
> > 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.
> 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.
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.


https://reviews.llvm.org/D32896