[PATCH] D32896: [OpenCL] Make CLK_NULL_RESERVE_ID invalid reserve id.

Thu Aug 10 07:53:00 PDT 2017

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 __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.
Sorry, I didn't update this code review thread, but I emailed to the working group regarding whether this macro should be implementation defined or set by the spec, since we need some portable binary representation for SPIR-V format. This thread didn't received any attention, but I think I found the right SPIR-V representation for this particular case.

@b-sumner brought another good question. It would be great if could resolve this ambiguity by removing CLK_NULL_RESERVE_ID macro.

https://reviews.llvm.org/D32896