yaxunl 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); ---------------- 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. https://reviews.llvm.org/D32896 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
