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. > > > > > > 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? > > > I was talking about SPIR in a general sense. What I said applies to > > > either SPIR or SPIR-V, as long as we want a portable representation. > > > Currently Clang does not support SPIR-V, but I guess the header file will > > > be used for SPIR-V in the future in the similar way as it is used for > > > SPIR. > > > > > > Yes, that is still an option. In that case I want a uniform compile-time > > > constant definition for all targets. > > > > > > If we choose to allow each target having its own constant value, then for > > > SPIR or SPIR-V it needs to be defined as a function. > > > What I said applies to either SPIR or SPIR-V, as long as we want a > > > portable representation. ... > > > If we choose to allow each target having its own constant value, then for > > > SPIR or SPIR-V it needs to be defined as a function. > > > > SPIR-V (as well as OpenCL) doesn't define CLK_NULL_RESERVE_ID, so it's not > > portable. Replacing it with function call puts additional implicit > > requirements for the frameworks consuming SPIR-V to provide > > __clk_null_reserve_id function implementation. > > > > > Yes, that is still an option. In that case I want a uniform compile-time > > > constant definition for all targets. > > > > So far, I'm in favor of this option. I think we need to bring this question > > to the Khronos and either: > > 1. define uniform compile-time constant > > 2. make the OpenCL value implementation defined and introduce SPIR-V > > representation for CLK_NULL_RESERVE_ID > Agree. I wish I understood why we have both CLK_NULL_RESERVE_ID and the function is_valid_reserve_id(). We don't need both, and the predicate makes the code clearer. Also, that macro implies that == and != must be defined for reserve_id_t which is again undesirable. I'd say any CLK_* value is automatically implementation defined if its value is not defined in the spec, but wouldn't object to clarifying with the working group. https://reviews.llvm.org/D32896 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
