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 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 target. > > Do you refer to SPIR-V or SPIR 2.0? BTW, you also mentioned that it's possible to up modify your implementation to align it with proposed version. > However, we can modify our implementation to adopt this change since it > brings about benefits overall. Is it still an option? https://reviews.llvm.org/D32896 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
