[Mesa-dev] [PATCH 7.5/11] glsl: Kill __intrinsic_atomic_sub

[Ian Romanick]

On 07/07/2016 06:34 PM, Ilia Mirkin wrote:
> On Thu, Jul 7, 2016 at 9:26 PM, Ian Romanick <idr at freedesktop.org> wrote:
>> On 07/07/2016 04:58 PM, Ilia Mirkin wrote:
>>> On Thu, Jul 7, 2016 at 5:02 PM, Ian Romanick <idr at freedesktop.org> wrote:
>>>> From: Ian Romanick <ian.d.romanick at intel.com>
>>>>
>>>> Just generate an __intrinsic_atomic_add with a negated parameter.
>>>>
>>>> Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
>>>> ---
>>>>  src/compiler/glsl/builtin_functions.cpp    | 50 +++++++++++++++++++++++++++---
>>>>  src/mesa/state_tracker/st_glsl_to_tgsi.cpp |  8 -----
>>>>  2 files changed, 46 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/src/compiler/glsl/builtin_functions.cpp b/src/compiler/glsl/builtin_functions.cpp
>>>> index 941ea12..ef3b2b0 100644
>>>> --- a/src/compiler/glsl/builtin_functions.cpp
>>>> +++ b/src/compiler/glsl/builtin_functions.cpp
>>>> @@ -3310,13 +3310,29 @@ builtin_builder::asin_expr(ir_variable *x, float p0, float p1)
>>>>                                            mul(abs(x), imm(p1))))))))));
>>>>  }
>>>>
>>>> +/**
>>>> + * Generate a ir_call to a function with a set of parameters
>>>> + *
>>>> + * The input \c params can either be a list of \c ir_variable or a list of
>>>> + * \c ir_dereference_variable.  In the latter case, all nodes will be removed
>>>> + * from \c params and used directly as the parameters to the generated
>>>> + * \c ir_call.
>>>> + */
>>>>  ir_call *
>>>>  builtin_builder::call(ir_function *f, ir_variable *ret, exec_list params)
>>>>  {
>>>>     exec_list actual_params;
>>>>
>>>> -   foreach_in_list(ir_variable, var, &params) {
>>>> -      actual_params.push_tail(var_ref(var));
>>>> +   foreach_in_list_safe(ir_instruction, ir, &params) {
>>>> +      ir_dereference_variable *d = ir->as_dereference_variable();
>>>> +      if (d != NULL) {
>>>> +         d->remove();
>>>> +         actual_params.push_tail(d);
>>>> +      } else {
>>>> +         ir_variable *var = ir->as_variable();
>>>> +         assert(var != NULL);
>>>> +         actual_params.push_tail(var_ref(var));
>>>> +      }
>>>>     }
>>>>
>>>>     ir_function_signature *sig =
>>>> @@ -5292,8 +5308,34 @@ builtin_builder::_atomic_counter_op1(const char *intrinsic,
>>>>     MAKE_SIG(glsl_type::uint_type, avail, 2, counter, data);
>>>>
>>>>     ir_variable *retval = body.make_temp(glsl_type::uint_type, "atomic_retval");
>>>> -   body.emit(call(shader->symbols->get_function(intrinsic), retval,
>>>> -                  sig->parameters));
>>>> +
>>>> +   /* Instead of generating an __intrinsic_atomic_sub, generate an
>>>> +    * __intrinsic_atomic_add with the data parameter negated.
>>>> +    */
>>>> +   if (strcmp("__intrinsic_atomic_sub", intrinsic) == 0) {
>>>> +      ir_variable *const neg_data =
>>>> +         body.make_temp(glsl_type::uint_type, "neg_data");
>>>> +
>>>> +      body.emit(assign(neg_data, neg(data)));
>>>> +
>>>> +      exec_list parameters;
>>>> +
>>>> +      parameters.push_tail(new(mem_ctx) ir_dereference_variable(counter));
>>>> +      parameters.push_tail(new(mem_ctx) ir_dereference_variable(neg_data));
>>>
>>> I don't get it ... why change call() to allow taking dereferences and
>>> create them here rather than just feeding in the ir_variables
>>> directly?
>>
>> Oh, I already went down that path.  :)  neg_data would have to be in two
>> lists at the same time:  the instruction stream and parameters.
>> Restructuring the code so that the ir_variables could be in parameters
>> then move them to the instruction stream was... enough to make a grown
>> Mick Jagger cry.
>>
>> I'm not terribly enamored with this solution either, but I didn't see a
>> better way.
> 
> How does it work in the "normal" case, i.e. if I just write GLSL that looks like
> 
> int foo = 1;
> bar(foo)
> 
> Is there a separate ir_variable created to hold the foo inside the
> call? If so, that seems easy enough too ... perhaps there's a
> non-obvious reason why that turns into a pile of sadness?

ir_call in the instruction stream has an exec_list that contains
ir_dereference_variable nodes.

The builtin_builder::call method previously took an exec_list of
ir_variables and created a list of ir_dereference_variable.  All of the
original users of that method wanted to make a function call using
exactly the set of parameters passed to the built-in function (i.e.,
call __intrinsic_atomic_add using the parameters to atomicAdd).  For
these users, the list of ir_variables already existed:  the list of
parameters in the built-in function signature.

This new caller doesn't do that.  It wants to call a function with a
parameter from the function and a value calculated in the function.  So,
I changed builtin_builder::call to take a list that could either be a
list of ir_variable or a list of ir_dereference_variable.  In the former
case it behaves just as it previously did.  In the latter case, it uses
(and removes from the input list) the ir_dereference_variable nodes
instead of creating new ones.