[Mesa-dev] [PATCH 2/3] intel/vec4: Fix nir_op_b2[fi] with 64-bit result

[Ian Romanick]

On 10/11/2018 11:36 AM, Jason Ekstrand wrote:
> On Thu, Oct 11, 2018 at 1:23 PM Ian Romanick <idr at freedesktop.org
> <mailto:idr at freedesktop.org>> wrote:
> 
>     On 10/11/2018 11:21 AM, Ian Romanick wrote:
>     > On 10/11/2018 08:38 AM, Jason Ekstrand wrote:
>     >> ---
>     >>  src/intel/compiler/brw_vec4_nir.cpp | 7 ++++++-
>     >>  1 file changed, 6 insertions(+), 1 deletion(-)
>     >>
>     >> diff --git a/src/intel/compiler/brw_vec4_nir.cpp
>     b/src/intel/compiler/brw_vec4_nir.cpp
>     >> index eaf1754b006..5ccfd1f8940 100644
>     >> --- a/src/intel/compiler/brw_vec4_nir.cpp
>     >> +++ b/src/intel/compiler/brw_vec4_nir.cpp
>     >> @@ -1586,7 +1586,12 @@ vec4_visitor::nir_emit_alu(nir_alu_instr
>     *instr)
>     >> 
>     >>     case nir_op_b2i:
>     >>     case nir_op_b2f:
>     >> -      emit(MOV(dst, negate(op[0])));
>     >> +      if (nir_dest_bit_size(instr->dest.dest) > 32) {
>     >> +         assert(dst.type == BRW_REGISTER_TYPE_DF);
>     >> +         emit_conversion_to_double(dst, negate(op[0]), false);
>     >> +      } else {
>     >> +         emit(MOV(dst, negate(op[0])));
>     >> +      }
>     >
>     > I wrote almost the same patch, but I split nir_op_b2i and nir_op_b2f
>     > handling and omitted the assert.  I was never able to get the
>     > then-clause to trigger.  Were you able to get that to happen?
> 
>     I guess the next patch causes it?  Assuming that's the case, you should
>     add that to the commit message for this patch.
> 
> 
> Correct.  Sure, I can add that to the commit message.  I was going to CC
> it to stable because I don't really know if it's possible to trigger or
> not.  It definitely does trigger with the previous patch but I can't
> prove that it doesn't trigger without it.  Thoughts?

Unless there's some way to do it from SPIR-V, I don't think it's
possible to trigger either the FS case or the vec4 case without at least
a68dd47b911.  Assuming that's true, I don't think any of these need to
go to stable.  GLSL IR doesn't have a bool-to-double opcode, so it will
always generate something like double(float(boolean_value)).  It might
be worth adding that to this commit message too.  The commit message for
patch 1 already implies this.

With whatever commit message changes you deem appropriate, this series is

Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>

>     >>        break;
>     >> 
>     >>     case nir_op_f2b:
>     >
>     > _______________________________________________
>     > mesa-dev mailing list
>     > mesa-dev at lists.freedesktop.org <mailto:mesa-dev at lists.freedesktop.org>
>     > https://lists.freedesktop.org/mailman/listinfo/mesa-dev