Re: [Mesa-dev] [PATCH] intel/eu: Don't apply chansel when repctrl is set

2018-10-22 Thread Sagar Ghuge 


On 10/22/18 10:14 AM, Matt Turner wrote:
> On Tue, Oct 16, 2018 at 4:57 PM Sagar Ghuge  wrote:
>>
> 
> Let's describe a little of why we're doing this and how we found it.
> If I recall correctly, we set a NOP (XYZW) swizzle on 3-src
> instructions, except we set an  swizzle on LRP. Is that correct?
> 

Yes. I will resend patch with full description. 

>> Signed-off-by: Sagar Ghuge 
>> ---
>>  src/intel/compiler/brw_eu_emit.c | 36 
>>  1 file changed, 27 insertions(+), 9 deletions(-)
>>
>> diff --git a/src/intel/compiler/brw_eu_emit.c 
>> b/src/intel/compiler/brw_eu_emit.c
>> index 0cbc682ebc..a6b45fcb1a 100644
>> --- a/src/intel/compiler/brw_eu_emit.c
>> +++ b/src/intel/compiler/brw_eu_emit.c
>> @@ -765,31 +765,49 @@ brw_alu3(struct brw_codegen *p, unsigned opcode, 
>> struct brw_reg dest,
>>brw_inst_set_3src_a16_dst_writemask(devinfo, inst, dest.writemask);
>>
>>assert(src0.file == BRW_GENERAL_REGISTER_FILE);
>> -  brw_inst_set_3src_a16_src0_swizzle(devinfo, inst, src0.swizzle);
>>brw_inst_set_3src_a16_src0_subreg_nr(devinfo, inst, 
>> get_3src_subreg_nr(src0));
>>brw_inst_set_3src_src0_reg_nr(devinfo, inst, src0.nr);
>>brw_inst_set_3src_src0_abs(devinfo, inst, src0.abs);
>>brw_inst_set_3src_src0_negate(devinfo, inst, src0.negate);
>> -  brw_inst_set_3src_a16_src0_rep_ctrl(devinfo, inst,
>> -  src0.vstride == 
>> BRW_VERTICAL_STRIDE_0);
>> +
>> +  /* RepCtrl field in Source or Destination Operand table in BDW Bspec
>> +   * says:
>> +   *"ChanSel does not apply when Replicate Control is set."
>> +   */
>> +  if (src0.vstride == BRW_VERTICAL_STRIDE_0)
>> + brw_inst_set_3src_a16_src0_rep_ctrl(devinfo, inst, true);
>> +  else
>> + brw_inst_set_3src_a16_src0_swizzle(devinfo, inst, src0.swizzle);
> 
> I see that the brw_vec1_reg() function uses an  swizzle. That
> indeed determines what swizzle scalar sources (i.e., those with
> rep_ctrl set) will have.
> 
> I would rather we always set the swizzle (so as to not complicate the
> code here) and instead figure out why the brw_reg's swizzle field is
> sometimes XYZW fix that.
> 
> I modified brw_disasm.c locally to always print the swizzle on 3-src
> operands and using the piglit tests
> 
> generated_tests/spec/glsl-1.10/execution/built-in-functions/fs-mix-vec2-vec2-vec2.shader_test
> (for LRP)
> tests/spec/arb_gpu_shader5/execution/built-in-functions/fs-fma.shader_test
> (for MAD)
> 
> I cannot confirm that the swizzles are different between MAD and LRP.
> What am I missing?
> 
> lrp(8)  g4<1>F  g2.4<0,1,0>.xF  g2.2<0,1,0>.xF
> g2.0<0,1,0>.xF { align16 1Q };
> mad(8)  g4<1>F  g3.0<0,1,0>.xF  g2.4<0,1,0>.xF
> g2.0<0,1,0>.xF { align16 1Q };
> 
My bad, swizzles are not different (I will investigate more with different 
test), 
but while assembling we set both the rep_ctrl 
and the swizzle bits without checking region is scalar or not, 

https://gitlab.freedesktop.org/mesa/mesa/blob/master/src/intel/compiler/brw_eu_emit.c#L768

and while disassembling we check that if the region is scalar g1<0,1,0> then we 
don't 
disassemble the swizzle bits. For src0/src1/src2 in 3src operand,

https://gitlab.freedesktop.org/mesa/mesa/blob/master/src/intel/compiler/brw_disasm.c#L1103

That's where we have different behavior for assembling and disassembling the 
instruction. 

BDW+ Bspec says RepCtrl : "This field is only present in three-source 
instructions, 
for each of the three source operands. It controls replication of the starting 
channel
to all channels in the execution size. ChanSel does not apply when Replicate 
Control is set."
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] intel/eu: Don't apply chansel when repctrl is set

2018-10-22 Thread Matt Turner 
On Tue, Oct 16, 2018 at 4:57 PM Sagar Ghuge  wrote:
>

Let's describe a little of why we're doing this and how we found it.
If I recall correctly, we set a NOP (XYZW) swizzle on 3-src
instructions, except we set an  swizzle on LRP. Is that correct?

> Signed-off-by: Sagar Ghuge 
> ---
>  src/intel/compiler/brw_eu_emit.c | 36 
>  1 file changed, 27 insertions(+), 9 deletions(-)
>
> diff --git a/src/intel/compiler/brw_eu_emit.c 
> b/src/intel/compiler/brw_eu_emit.c
> index 0cbc682ebc..a6b45fcb1a 100644
> --- a/src/intel/compiler/brw_eu_emit.c
> +++ b/src/intel/compiler/brw_eu_emit.c
> @@ -765,31 +765,49 @@ brw_alu3(struct brw_codegen *p, unsigned opcode, struct 
> brw_reg dest,
>brw_inst_set_3src_a16_dst_writemask(devinfo, inst, dest.writemask);
>
>assert(src0.file == BRW_GENERAL_REGISTER_FILE);
> -  brw_inst_set_3src_a16_src0_swizzle(devinfo, inst, src0.swizzle);
>brw_inst_set_3src_a16_src0_subreg_nr(devinfo, inst, 
> get_3src_subreg_nr(src0));
>brw_inst_set_3src_src0_reg_nr(devinfo, inst, src0.nr);
>brw_inst_set_3src_src0_abs(devinfo, inst, src0.abs);
>brw_inst_set_3src_src0_negate(devinfo, inst, src0.negate);
> -  brw_inst_set_3src_a16_src0_rep_ctrl(devinfo, inst,
> -  src0.vstride == 
> BRW_VERTICAL_STRIDE_0);
> +
> +  /* RepCtrl field in Source or Destination Operand table in BDW Bspec
> +   * says:
> +   *"ChanSel does not apply when Replicate Control is set."
> +   */
> +  if (src0.vstride == BRW_VERTICAL_STRIDE_0)
> + brw_inst_set_3src_a16_src0_rep_ctrl(devinfo, inst, true);
> +  else
> + brw_inst_set_3src_a16_src0_swizzle(devinfo, inst, src0.swizzle);

I see that the brw_vec1_reg() function uses an  swizzle. That
indeed determines what swizzle scalar sources (i.e., those with
rep_ctrl set) will have.

I would rather we always set the swizzle (so as to not complicate the
code here) and instead figure out why the brw_reg's swizzle field is
sometimes XYZW fix that.

I modified brw_disasm.c locally to always print the swizzle on 3-src
operands and using the piglit tests

generated_tests/spec/glsl-1.10/execution/built-in-functions/fs-mix-vec2-vec2-vec2.shader_test
(for LRP)
tests/spec/arb_gpu_shader5/execution/built-in-functions/fs-fma.shader_test
(for MAD)

I cannot confirm that the swizzles are different between MAD and LRP.
What am I missing?

lrp(8)  g4<1>F  g2.4<0,1,0>.xF  g2.2<0,1,0>.xF
g2.0<0,1,0>.xF { align16 1Q };
mad(8)  g4<1>F  g3.0<0,1,0>.xF  g2.4<0,1,0>.xF
g2.0<0,1,0>.xF { align16 1Q };
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] intel/eu: Don't apply chansel when repctrl is set

2018-10-16 Thread Sagar Ghuge 
Signed-off-by: Sagar Ghuge 
---
 src/intel/compiler/brw_eu_emit.c | 36 
 1 file changed, 27 insertions(+), 9 deletions(-)

diff --git a/src/intel/compiler/brw_eu_emit.c b/src/intel/compiler/brw_eu_emit.c
index 0cbc682ebc..a6b45fcb1a 100644
--- a/src/intel/compiler/brw_eu_emit.c
+++ b/src/intel/compiler/brw_eu_emit.c
@@ -765,31 +765,49 @@ brw_alu3(struct brw_codegen *p, unsigned opcode, struct 
brw_reg dest,
   brw_inst_set_3src_a16_dst_writemask(devinfo, inst, dest.writemask);
 
   assert(src0.file == BRW_GENERAL_REGISTER_FILE);
-  brw_inst_set_3src_a16_src0_swizzle(devinfo, inst, src0.swizzle);
   brw_inst_set_3src_a16_src0_subreg_nr(devinfo, inst, 
get_3src_subreg_nr(src0));
   brw_inst_set_3src_src0_reg_nr(devinfo, inst, src0.nr);
   brw_inst_set_3src_src0_abs(devinfo, inst, src0.abs);
   brw_inst_set_3src_src0_negate(devinfo, inst, src0.negate);
-  brw_inst_set_3src_a16_src0_rep_ctrl(devinfo, inst,
-  src0.vstride == 
BRW_VERTICAL_STRIDE_0);
+
+  /* RepCtrl field in Source or Destination Operand table in BDW Bspec
+   * says:
+   *"ChanSel does not apply when Replicate Control is set."
+   */
+  if (src0.vstride == BRW_VERTICAL_STRIDE_0)
+ brw_inst_set_3src_a16_src0_rep_ctrl(devinfo, inst, true);
+  else
+ brw_inst_set_3src_a16_src0_swizzle(devinfo, inst, src0.swizzle);
 
   assert(src1.file == BRW_GENERAL_REGISTER_FILE);
-  brw_inst_set_3src_a16_src1_swizzle(devinfo, inst, src1.swizzle);
   brw_inst_set_3src_a16_src1_subreg_nr(devinfo, inst, 
get_3src_subreg_nr(src1));
   brw_inst_set_3src_src1_reg_nr(devinfo, inst, src1.nr);
   brw_inst_set_3src_src1_abs(devinfo, inst, src1.abs);
   brw_inst_set_3src_src1_negate(devinfo, inst, src1.negate);
-  brw_inst_set_3src_a16_src1_rep_ctrl(devinfo, inst,
-  src1.vstride == 
BRW_VERTICAL_STRIDE_0);
+
+  /* RepCtrl field in Source or Destination Operand table in BDW Bspec
+   * says:
+   *"ChanSel does not apply when Replicate Control is set."
+   */
+  if (src1.vstride == BRW_VERTICAL_STRIDE_0)
+ brw_inst_set_3src_a16_src1_rep_ctrl(devinfo, inst, true);
+  else
+ brw_inst_set_3src_a16_src1_swizzle(devinfo, inst, src1.swizzle);
 
   assert(src2.file == BRW_GENERAL_REGISTER_FILE);
-  brw_inst_set_3src_a16_src2_swizzle(devinfo, inst, src2.swizzle);
   brw_inst_set_3src_a16_src2_subreg_nr(devinfo, inst, 
get_3src_subreg_nr(src2));
   brw_inst_set_3src_src2_reg_nr(devinfo, inst, src2.nr);
   brw_inst_set_3src_src2_abs(devinfo, inst, src2.abs);
   brw_inst_set_3src_src2_negate(devinfo, inst, src2.negate);
-  brw_inst_set_3src_a16_src2_rep_ctrl(devinfo, inst,
-  src2.vstride == 
BRW_VERTICAL_STRIDE_0);
+
+  /* RepCtrl field in Source or Destination Operand table in BDW Bspec
+   * says:
+   *"ChanSel does not apply when Replicate Control is set."
+   */
+  if (src2.vstride == BRW_VERTICAL_STRIDE_0)
+ brw_inst_set_3src_a16_src2_rep_ctrl(devinfo, inst, true);
+  else
+ brw_inst_set_3src_a16_src2_swizzle(devinfo, inst, src2.swizzle);
 
   if (devinfo->gen >= 7) {
  /* Set both the source and destination types based on dest.type,
-- 
2.17.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev