[PATCH] D32896: [OpenCL] Make CLK_NULL_RESERVE_ID invalid reserve id.
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); b-sumner wrote: > yaxunl wrote: > > bader wrote: > > > yaxunl wrote: > > > > bader wrote: > > > > > yaxunl wrote: > > > > > > bader wrote: > > > > > > > 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 > > > > > > > > >
[PATCH] D32896: [OpenCL] Make CLK_NULL_RESERVE_ID invalid reserve id.
b-sumner 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: > bader wrote: > > yaxunl wrote: > > > bader wrote: > > > > yaxunl wrote: > > > > > bader wrote: > > > > > > 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. > > > > >
[PATCH] D32896: [OpenCL] Make CLK_NULL_RESERVE_ID invalid reserve id.
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: > > bader wrote: > > > 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
[PATCH] D32896: [OpenCL] Make CLK_NULL_RESERVE_ID invalid reserve id.
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: > bader wrote: > > 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? > No. That makes us unable to consume SPIR since
[PATCH] D32896: [OpenCL] Make CLK_NULL_RESERVE_ID invalid reserve id.
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: > > 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? 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
[PATCH] D32896: [OpenCL] Make CLK_NULL_RESERVE_ID invalid reserve id.
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
[PATCH] D32896: [OpenCL] Make CLK_NULL_RESERVE_ID invalid reserve id.
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); 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. https://reviews.llvm.org/D32896 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32896: [OpenCL] Make CLK_NULL_RESERVE_ID invalid reserve id.
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
[PATCH] D32896: [OpenCL] Make CLK_NULL_RESERVE_ID invalid reserve id.
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 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32896: [OpenCL] Make CLK_NULL_RESERVE_ID invalid reserve id.
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); 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. https://reviews.llvm.org/D32896 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32896: [OpenCL] Make CLK_NULL_RESERVE_ID invalid reserve id.
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 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32896: [OpenCL] Make CLK_NULL_RESERVE_ID invalid reserve id.
echuraev 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: > > 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. https://reviews.llvm.org/D32896 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32896: [OpenCL] Make CLK_NULL_RESERVE_ID invalid reserve id.
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); 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. https://reviews.llvm.org/D32896 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32896: [OpenCL] Make CLK_NULL_RESERVE_ID invalid reserve id.
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); 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. https://reviews.llvm.org/D32896 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32896: [OpenCL] Make CLK_NULL_RESERVE_ID invalid reserve id.
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); 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? https://reviews.llvm.org/D32896 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32896: [OpenCL] Make CLK_NULL_RESERVE_ID invalid reserve id.
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); 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`. https://reviews.llvm.org/D32896 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32896: [OpenCL] Make CLK_NULL_RESERVE_ID invalid reserve id.
echuraev created this revision. Herald added a subscriber: yaxunl. 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. https://reviews.llvm.org/D32896 Files: lib/Headers/opencl-c.h Index: lib/Headers/opencl-c.h === --- lib/Headers/opencl-c.h +++ lib/Headers/opencl-c.h @@ -16014,8 +16014,10 @@ // 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 Index: lib/Headers/opencl-c.h === --- lib/Headers/opencl-c.h +++ lib/Headers/opencl-c.h @@ -16014,8 +16014,10 @@ // 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 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
