[PATCH] MIPS: Fix mode mismatch error between Loongson builtin arguments and insn operands.

[PATCH] MIPS: Fix mode mismatch error between Loongson builtin arguments and insn operands.

[Toma Tabacu]

Hi,

The builtins for the pshufh, psllh, psllw, psrah, psraw, psrlh, psrlw Loongson
instructions have the third argument's type set to UQI while its corresponding
insn operand is in SImode.

This results in the following error when matching insn operands:

../gcc/gcc/include/loongson.h: In function 'test_psllw_s':
../gcc/gcc/include/loongson.h:483:10: error: invalid argument to built-in function
   return __builtin_loongson_psllw_s (s, amount);
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

This causes the loongson-simd.c and loongson-shift-count-truncated-1.c tests
to fail.

This patch fixes this by wrapping the QImode builtin argument inside a
paradoxical SUBREG with SImode, which will successfully match against the insn
operand.

Tested with mips-mti-elf.

Regards,
Toma

gcc/

	* config/mips/mips.c (mips_expand_builtin_insn): Put the QImode
	argument of the pshufh, psllh, psllw, psrah, psraw, psrlh, psrlw
	builtins into an SImode paradoxical SUBREG.

diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c
index da7fa8f..f1ca6e2 100644
--- a/gcc/config/mips/mips.c
+++ b/gcc/config/mips/mips.c
@@ -16574,6 +16574,20 @@ mips_expand_builtin_insn (enum insn_code icode, unsigned int nops,
 
   switch (icode)
     {
+    /* The third argument needs to be in SImode in order to succesfully match
+       the operand from the insn definition.  */
+    case CODE_FOR_loongson_pshufh:
+    case CODE_FOR_loongson_psllh:
+    case CODE_FOR_loongson_psllw:
+    case CODE_FOR_loongson_psrah:
+    case CODE_FOR_loongson_psraw:
+    case CODE_FOR_loongson_psrlh:
+    case CODE_FOR_loongson_psrlw:
+      gcc_assert (has_target_p && nops == 3 && ops[2].mode == QImode);
+      ops[2].value = lowpart_subreg (SImode, ops[2].value, QImode);
+      ops[2].mode = SImode;
+      break;
+
     case CODE_FOR_msa_addvi_b:
     case CODE_FOR_msa_addvi_h:
     case CODE_FOR_msa_addvi_w:

RE: [PATCH] MIPS: Fix mode mismatch error between Loongson builtin arguments and insn operands.

[Matthew Fortune]

Toma Tabacu <Toma.Tabacu@imgtec.com> writes:
> The builtins for the pshufh, psllh, psllw, psrah, psraw, psrlh, psrlw
> Loongson instructions have the third argument's type set to UQI while
> its corresponding insn operand is in SImode.
> 
> This results in the following error when matching insn operands:
> 
> ../gcc/gcc/include/loongson.h: In function 'test_psllw_s':
> ../gcc/gcc/include/loongson.h:483:10: error: invalid argument to built-
> in function
>    return __builtin_loongson_psllw_s (s, amount);
>           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> This causes the loongson-simd.c and loongson-shift-count-truncated-1.c
> tests to fail.
> 
> This patch fixes this by wrapping the QImode builtin argument inside a
> paradoxical SUBREG with SImode, which will successfully match against
> the insn operand.
> 
> Tested with mips-mti-elf.
> 
> Regards,
> Toma
> 
> gcc/
> 
> 	* config/mips/mips.c (mips_expand_builtin_insn): Put the QImode
> 	argument of the pshufh, psllh, psllw, psrah, psraw, psrlh, psrlw
> 	builtins into an SImode paradoxical SUBREG.
> 
> diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c index
> da7fa8f..f1ca6e2 100644
> --- a/gcc/config/mips/mips.c
> +++ b/gcc/config/mips/mips.c
> @@ -16574,6 +16574,20 @@ mips_expand_builtin_insn (enum insn_code icode,
> unsigned int nops,
> 
>    switch (icode)
>      {
> +    /* The third argument needs to be in SImode in order to succesfully
> match
> +       the operand from the insn definition.  */

Please refer to operand here not argument as it is the second argument
to the builtin but third operand of the instruction.  Also double ss in 
successfully.

> +    case CODE_FOR_loongson_pshufh:
> +    case CODE_FOR_loongson_psllh:
> +    case CODE_FOR_loongson_psllw:
> +    case CODE_FOR_loongson_psrah:
> +    case CODE_FOR_loongson_psraw:
> +    case CODE_FOR_loongson_psrlh:
> +    case CODE_FOR_loongson_psrlw:
> +      gcc_assert (has_target_p && nops == 3 && ops[2].mode == QImode);
> +      ops[2].value = lowpart_subreg (SImode, ops[2].value, QImode);
> +      ops[2].mode = SImode;
> +      break;
> +
>      case CODE_FOR_msa_addvi_b:
>      case CODE_FOR_msa_addvi_h:
>      case CODE_FOR_msa_addvi_w:

For the record, given paradoxical subregs are a headache...
I am OK with this on the basis that the argument to psllh etc is actually
a uint8_t which means that bits 8 upwards are guaranteed to be zero so
the subreg can be eliminated without any explicit sign or zero extension
inserted.  This is the same kind of optimisation that combine would
perform when eliminating zero extension.

Please can you check that a zero extension is inserted for the following
case with -O2 or above:

int16x4_t testme(int16x4_t s, int amount)
{
  return psllh_s (s, amount);
}

If my understanding is correct there should be an ANDI 0xff inserted
or similar.

OK with those changes and confirmation of the test above.

Thanks,
Matthew

RE: [PATCH] MIPS: Fix mode mismatch error between Loongson builtin arguments and insn operands.

[Toma Tabacu]

> From: Matthew Fortune
> > +    /* The third argument needs to be in SImode in order to succesfully
> > match
> > +       the operand from the insn definition.  */
> 
> Please refer to operand here not argument as it is the second argument
> to the builtin but third operand of the instruction.  Also double ss in
> successfully.
> 

I have rewritten the comment to address these mistakes.

> > +    case CODE_FOR_loongson_pshufh:
> > +    case CODE_FOR_loongson_psllh:
> > +    case CODE_FOR_loongson_psllw:
> > +    case CODE_FOR_loongson_psrah:
> > +    case CODE_FOR_loongson_psraw:
> > +    case CODE_FOR_loongson_psrlh:
> > +    case CODE_FOR_loongson_psrlw:
> > +      gcc_assert (has_target_p && nops == 3 && ops[2].mode == QImode);
> > +      ops[2].value = lowpart_subreg (SImode, ops[2].value, QImode);
> > +      ops[2].mode = SImode;
> > +      break;
> > +
> >      case CODE_FOR_msa_addvi_b:
> >      case CODE_FOR_msa_addvi_h:
> >      case CODE_FOR_msa_addvi_w:
> 
> For the record, given paradoxical subregs are a headache...
> I am OK with this on the basis that the argument to psllh etc is actually
> a uint8_t which means that bits 8 upwards are guaranteed to be zero so
> the subreg can be eliminated without any explicit sign or zero extension
> inserted.  This is the same kind of optimisation that combine would
> perform when eliminating zero extension.
> 
> Please can you check that a zero extension is inserted for the following
> case with -O2 or above:
> 
> int16x4_t testme(int16x4_t s, int amount)
> {
>   return psllh_s (s, amount);
> }
> 
> If my understanding is correct there should be an ANDI 0xff inserted
> or similar.
> 

The ANDI 0xff is present for -O0, after the first time the value is loaded
from memory, but it is not generated for -O1 and -O2.
I'm not seeing any zero extension happening for -O1 and -O2.

The only change in the patch below is the fixed comment.

Regards,
Toma

gcc/

	* config/mips/mips.c (mips_expand_builtin_insn): Put the QImode
	argument of the pshufh, psllh, psllw, psrah, psraw, psrlh, psrlw
	builtins into an SImode paradoxical SUBREG.

diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c
index da7fa8f..e5b2d9a 100644
--- a/gcc/config/mips/mips.c
+++ b/gcc/config/mips/mips.c
@@ -16574,6 +16574,20 @@ mips_expand_builtin_insn (enum insn_code icode, unsigned int nops,
 
   switch (icode)
     {
+    /* The third operand of these instructions is in SImode, so we need to
+       bring the corresponding builtin argument from QImode into SImode.  */
+    case CODE_FOR_loongson_pshufh:
+    case CODE_FOR_loongson_psllh:
+    case CODE_FOR_loongson_psllw:
+    case CODE_FOR_loongson_psrah:
+    case CODE_FOR_loongson_psraw:
+    case CODE_FOR_loongson_psrlh:
+    case CODE_FOR_loongson_psrlw:
+      gcc_assert (has_target_p && nops == 3 && ops[2].mode == QImode);
+      ops[2].value = lowpart_subreg (SImode, ops[2].value, QImode);
+      ops[2].mode = SImode;
+      break;
+
     case CODE_FOR_msa_addvi_b:
     case CODE_FOR_msa_addvi_h:
     case CODE_FOR_msa_addvi_w:

RE: [PATCH] MIPS: Fix mode mismatch error between Loongson builtin arguments and insn operands.

[Matthew Fortune]

Toma Tabacu <Toma.Tabacu@imgtec.com> writes:
> > From: Matthew Fortune
> > > +    /* The third argument needs to be in SImode in order to succesfully
> > > match
> > > +       the operand from the insn definition.  */
> >
> > Please refer to operand here not argument as it is the second argument
> > to the builtin but third operand of the instruction.  Also double ss in
> > successfully.
> >
> 
> I have rewritten the comment to address these mistakes.
> 
> > > +    case CODE_FOR_loongson_pshufh:
> > > +    case CODE_FOR_loongson_psllh:
> > > +    case CODE_FOR_loongson_psllw:
> > > +    case CODE_FOR_loongson_psrah:
> > > +    case CODE_FOR_loongson_psraw:
> > > +    case CODE_FOR_loongson_psrlh:
> > > +    case CODE_FOR_loongson_psrlw:
> > > +      gcc_assert (has_target_p && nops == 3 && ops[2].mode == QImode);
> > > +      ops[2].value = lowpart_subreg (SImode, ops[2].value, QImode);
> > > +      ops[2].mode = SImode;
> > > +      break;
> > > +
> > >      case CODE_FOR_msa_addvi_b:
> > >      case CODE_FOR_msa_addvi_h:
> > >      case CODE_FOR_msa_addvi_w:
> >
> > For the record, given paradoxical subregs are a headache...
> > I am OK with this on the basis that the argument to psllh etc is actually
> > a uint8_t which means that bits 8 upwards are guaranteed to be zero so
> > the subreg can be eliminated without any explicit sign or zero extension
> > inserted.  This is the same kind of optimisation that combine would
> > perform when eliminating zero extension.
> >
> > Please can you check that a zero extension is inserted for the following
> > case with -O2 or above:
> >
> > int16x4_t testme(int16x4_t s, int amount)
> > {
> >   return psllh_s (s, amount);
> > }
> >
> > If my understanding is correct there should be an ANDI 0xff inserted
> > or similar.
> >
> 
> The ANDI 0xff is present for -O0, after the first time the value is loaded
> from memory, but it is not generated for -O1 and -O2.
> I'm not seeing any zero extension happening for -O1 and -O2.

That's not what I hoped but is what I was concerned about as I believe it
means we have a change of behaviour.  It boils down to simply ignoring the
argument type of unsigned char.  My guess is that a zero extension is
created but then immediately eliminated because of the paradoxical subreg.

I think you need to create a temporary and perform the zero extension to
ensure we honour the unsigned char operand:

  rtx new_dst = gen_reg_rtx (SImode);
  emit_insn (gen_zero_extendqisi2 (new_dst, ops[2].value));
  ops[2].value = foo;

This should mean that the testcase I sent always has a zero extension but if
you change the type of 'amount' to be unsigned char then there should not be
a zero extension as the argument will be assumed to be correctly zero extended
already and the explicitly introduced zero_extend will be eliminated.

Apologies for not proposing this before.

Thanks,
Matthew

> 
> The only change in the patch below is the fixed comment.
> 
> Regards,
> Toma
> 
> gcc/
> 
> 	* config/mips/mips.c (mips_expand_builtin_insn): Put the QImode
> 	argument of the pshufh, psllh, psllw, psrah, psraw, psrlh, psrlw
> 	builtins into an SImode paradoxical SUBREG.
> 
> diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c
> index da7fa8f..e5b2d9a 100644
> --- a/gcc/config/mips/mips.c
> +++ b/gcc/config/mips/mips.c
> @@ -16574,6 +16574,20 @@ mips_expand_builtin_insn (enum insn_code icode, unsigned int
> nops,
> 
>    switch (icode)
>      {
> +    /* The third operand of these instructions is in SImode, so we need to
> +       bring the corresponding builtin argument from QImode into SImode.  */
> +    case CODE_FOR_loongson_pshufh:
> +    case CODE_FOR_loongson_psllh:
> +    case CODE_FOR_loongson_psllw:
> +    case CODE_FOR_loongson_psrah:
> +    case CODE_FOR_loongson_psraw:
> +    case CODE_FOR_loongson_psrlh:
> +    case CODE_FOR_loongson_psrlw:
> +      gcc_assert (has_target_p && nops == 3 && ops[2].mode == QImode);
> +      ops[2].value = lowpart_subreg (SImode, ops[2].value, QImode);
> +      ops[2].mode = SImode;
> +      break;
> +
>      case CODE_FOR_msa_addvi_b:
>      case CODE_FOR_msa_addvi_h:
>      case CODE_FOR_msa_addvi_w:

RE: [PATCH] MIPS: Fix mode mismatch error between Loongson builtin arguments and insn operands.

[Toma Tabacu]

Matthew Fortune writes:
> 
> That's not what I hoped but is what I was concerned about as I believe it
> means we have a change of behaviour.  It boils down to simply ignoring the
> argument type of unsigned char.  My guess is that a zero extension is
> created but then immediately eliminated because of the paradoxical subreg.
> 
> I think you need to create a temporary and perform the zero extension to
> ensure we honour the unsigned char operand:
> 
>   rtx new_dst = gen_reg_rtx (SImode);
>   emit_insn (gen_zero_extendqisi2 (new_dst, ops[2].value));
>   ops[2].value = foo;
> 
> This should mean that the testcase I sent always has a zero extension but if
> you change the type of 'amount' to be unsigned char then there should not be
> a zero extension as the argument will be assumed to be correctly zero extended
> already and the explicitly introduced zero_extend will be eliminated.
> 

I have made it generate a zero_extend instead of a SUBREG.
However, the pattern associated with gen_zero_extendqisi2 does not work with
immediate operands, so I had to add an extra step in which the argument is put
into a QImode register before being passed to gen_zero_extendqisi2.

Is this OK ?

Regards,
Toma

gcc/

	* config/mips/mips.c (mips_expand_builtin_insn): Convert the QImode
	argument of the pshufh, psllh, psllw, psrah, psraw, psrlh, psrlw
	builtins to SImode and emit a zero-extend, if necessary.

diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c
index da7fa8f..bab5b93 100644
--- a/gcc/config/mips/mips.c
+++ b/gcc/config/mips/mips.c
@@ -16571,9 +16571,35 @@ mips_expand_builtin_insn (enum insn_code icode, unsigned int nops,
 {
   machine_mode imode;
   int rangelo = 0, rangehi = 0, error_opno = 0;
+  rtx qireg, sireg;
 
   switch (icode)
     {
+    /* The third operand of these instructions is in SImode, so we need to
+       bring the corresponding builtin argument from QImode into SImode.  */
+    case CODE_FOR_loongson_pshufh:
+    case CODE_FOR_loongson_psllh:
+    case CODE_FOR_loongson_psllw:
+    case CODE_FOR_loongson_psrah:
+    case CODE_FOR_loongson_psraw:
+    case CODE_FOR_loongson_psrlh:
+    case CODE_FOR_loongson_psrlw:
+      gcc_assert (has_target_p && nops == 3 && ops[2].mode == QImode);
+      sireg = gen_reg_rtx (SImode);
+      /* We need to put the immediate in a register because
+	 gen_zero_extendqisi2 does not accept immediate operands.  */
+      if (CONST_INT_P (ops[2].value))
+	{
+	  qireg = gen_reg_rtx (QImode);
+	  emit_insn (gen_rtx_SET (qireg, ops[2].value));
+	  emit_insn (gen_zero_extendqisi2 (sireg, qireg));
+	} else {
+	  emit_insn (gen_zero_extendqisi2 (sireg, ops[2].value));
+	}
+      ops[2].value = sireg;
+      ops[2].mode = SImode;
+      break;
+
     case CODE_FOR_msa_addvi_b:
     case CODE_FOR_msa_addvi_h:
     case CODE_FOR_msa_addvi_w:

RE: [PATCH] MIPS: Fix mode mismatch error between Loongson builtin arguments and insn operands.

[Matthew Fortune]

Toma Tabacu <Toma.Tabacu@imgtec.com> writes:
> Matthew Fortune writes:
> >
> > That's not what I hoped but is what I was concerned about as I believe
> > it means we have a change of behaviour.  It boils down to simply
> > ignoring the argument type of unsigned char.  My guess is that a zero
> > extension is created but then immediately eliminated because of the
> paradoxical subreg.
> >
> > I think you need to create a temporary and perform the zero extension
> > to ensure we honour the unsigned char operand:
> >
> >   rtx new_dst = gen_reg_rtx (SImode);
> >   emit_insn (gen_zero_extendqisi2 (new_dst, ops[2].value));
> >   ops[2].value = foo;
> >
> > This should mean that the testcase I sent always has a zero extension
> > but if you change the type of 'amount' to be unsigned char then there
> > should not be a zero extension as the argument will be assumed to be
> > correctly zero extended already and the explicitly introduced
> zero_extend will be eliminated.
> >
> 
> I have made it generate a zero_extend instead of a SUBREG.
> However, the pattern associated with gen_zero_extendqisi2 does not work
> with immediate operands, so I had to add an extra step in which the
> argument is put into a QImode register before being passed to
> gen_zero_extendqisi2.
> 
> Is this OK ?
> 
> Regards,
> Toma
> 
> gcc/
> 
> 	* config/mips/mips.c (mips_expand_builtin_insn): Convert the QImode
> 	argument of the pshufh, psllh, psllw, psrah, psraw, psrlh, psrlw
> 	builtins to SImode and emit a zero-extend, if necessary.
> 
> diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c index
> da7fa8f..bab5b93 100644
> --- a/gcc/config/mips/mips.c
> +++ b/gcc/config/mips/mips.c
> @@ -16571,9 +16571,35 @@ mips_expand_builtin_insn (enum insn_code icode,
> unsigned int nops,  {
>    machine_mode imode;
>    int rangelo = 0, rangehi = 0, error_opno = 0;
> +  rtx qireg, sireg;
> 
>    switch (icode)
>      {
> +    /* The third operand of these instructions is in SImode, so we need
> to
> +       bring the corresponding builtin argument from QImode into
> SImode.  */
> +    case CODE_FOR_loongson_pshufh:
> +    case CODE_FOR_loongson_psllh:
> +    case CODE_FOR_loongson_psllw:
> +    case CODE_FOR_loongson_psrah:
> +    case CODE_FOR_loongson_psraw:
> +    case CODE_FOR_loongson_psrlh:
> +    case CODE_FOR_loongson_psrlw:
> +      gcc_assert (has_target_p && nops == 3 && ops[2].mode == QImode);
> +      sireg = gen_reg_rtx (SImode);
> +      /* We need to put the immediate in a register because
> +	 gen_zero_extendqisi2 does not accept immediate operands.  */
> +      if (CONST_INT_P (ops[2].value))
> +	{
> +	  qireg = gen_reg_rtx (QImode);
> +	  emit_insn (gen_rtx_SET (qireg, ops[2].value));
> +	  emit_insn (gen_zero_extendqisi2 (sireg, qireg));
> +	} else {
> +	  emit_insn (gen_zero_extendqisi2 (sireg, ops[2].value));
> +	}

Almost but not quite. There is a force_reg helper that takes care of
this i.e. can get rid of the qireg local and the whole if statement.

      emit_insn (gen_zero_extendqisi2 (sireg, force_reg (ops[2].value)));

> +      ops[2].value = sireg;
> +      ops[2].mode = SImode;
> +      break;
> +
>      case CODE_FOR_msa_addvi_b:
>      case CODE_FOR_msa_addvi_h:
>      case CODE_FOR_msa_addvi_w:

OK with that change.

Thanks,
Matthew

RE: [PATCH] MIPS: Fix mode mismatch error between Loongson builtin arguments and insn operands.

[Toma Tabacu]

Matthew Fortune writes:
> 
> Almost but not quite. There is a force_reg helper that takes care of
> this i.e. can get rid of the qireg local and the whole if statement.
> 
>       emit_insn (gen_zero_extendqisi2 (sireg, force_reg (ops[2].value)));
> 
> OK with that change.
> 
> Thanks,
> Matthew

Committed as r245243.

Thanks,
Toma