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:
> yaxunl wrote:
> > bader wrote:
> > > 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.
> > 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.
> Or we only do this for SPIR and define it as target specific value for other 
> targets.
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?


https://reviews.llvm.org/D32896



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

Reply via email to