[v2] MIPS: Output $0 for conditional trap if !ISA_HAS_COND_TRAPI

MIPSr6 removes condition trap instructions with imm, so the instruction
like `teq $2,imm` will be converted to
  li $at, imm
  teq $2, $at

The current version of Gas cannot detect if imm is zero, and output
  teq $2, $0
Let's do it in GCC.

gcc
	* config/mips/mips.md(conditional_trap_reg): Output $0 instead
	of 0 if !ISA_HAS_COND_TRAPI.
---
 gcc/config/mips/mips.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

On Thu, 20 Jun 2024, YunQiang Su wrote:

> MIPSr6 removes condition trap instructions with imm, so the instruction
> like `teq $2,imm` will be converted to
>   li $at, imm
>   teq $2, $at
> 
> The current version of Gas cannot detect if imm is zero, and output
>   teq $2, $0
> Let's do it in GCC.

 This description should state that the change is a fix for an actual bug 
in GCC where the output pattern does not match the constraints supplied, 
and what consequences this has that the fix addressed.  There is no `imm' 
in the general sense here, just the special case of zero.

 The missed optimisation in GAS, which used not to trigger pre-R6, is 
irrelevant from this change's point of view and just adds noise.  I'm 
surprised that it worked even in the first place, as I reckon GCC is 
supposed to emit regular MIPS code in the `.set nomacro' mode nowadays, 
which is the only way to guarantee that instruction lengths known to GCC 
do not accidentally disagree with what the assembler has produced, such 
as in the case of the bug your change has addressed.

 Overall ISTM there is no need for distinct insns for ISA_HAS_COND_TRAPI
and !ISA_HAS_COND_TRAPI cases each and this would better be sorted with 
predicates and constraints, especially as the output pattern is the same 
in both cases anyway.  This would prevent special-casing from being needed 
in `mips_expand_conditional_trap' as well.

  Maciej

Maciej W. Rozycki <macro@orcam.me.uk> 于2024年6月27日周四 00:07写道：
>
> On Thu, 20 Jun 2024, YunQiang Su wrote:
>
> > MIPSr6 removes condition trap instructions with imm, so the instruction
> > like `teq $2,imm` will be converted to
> >   li $at, imm
> >   teq $2, $at
> >
> > The current version of Gas cannot detect if imm is zero, and output
> >   teq $2, $0
> > Let's do it in GCC.
>
>  This description should state that the change is a fix for an actual bug
> in GCC where the output pattern does not match the constraints supplied,
> and what consequences this has that the fix addressed.  There is no `imm'
> in the general sense here, just the special case of zero.
>
>  The missed optimisation in GAS, which used not to trigger pre-R6, is
> irrelevant from this change's point of view and just adds noise.  I'm
> surprised that it worked even in the first place, as I reckon GCC is
> supposed to emit regular MIPS code in the `.set nomacro' mode nowadays,

In fact, GCC works well if IMM is not zero in mips_expand_conditional_trap

  mode = GET_MODE (XEXP (comparison, 0));
  op0 = force_reg (mode, op0);
  if (!(ISA_HAS_COND_TRAPI
        ? arith_operand (op1, mode)
        : reg_or_0_operand (op1, mode)))
    op1 = force_reg (mode, op1);              // <--------- here

This problem happens due to that GCC trust GAS so much ;)
It believe that GAS can recognize `TEQ $2,0`.


> which is the only way to guarantee that instruction lengths known to GCC
> do not accidentally disagree with what the assembler has produced, such
> as in the case of the bug your change has addressed.
>
>  Overall ISTM there is no need for distinct insns for ISA_HAS_COND_TRAPI
> and !ISA_HAS_COND_TRAPI cases each and this would better be sorted with
> predicates and constraints, especially as the output pattern is the same
> in both cases anyway.  This would prevent special-casing from being needed
> in `mips_expand_conditional_trap' as well.
>

I agree. The patch should be quite simple

   [(trap_if (match_operator:GPR 0 "trap_comparison_operator"
                                [(match_operand:GPR 1 "reg_or_0_operand" "dJ")
                                 (match_operand:GPR 2 "arith_operand" "dI")])
            (const_int 0))]
   "ISA_HAS_COND_TRAPI"
-  "t%C0\t%z1,%2"
+  "t%C0\t%z1,%z2"
   [(set_attr "type" "trap")])

I haven't do so, due to that I am wondering whether they have some
performance difference.

>   Maciej

On Thu, 27 Jun 2024, YunQiang Su wrote:

> >  The missed optimisation in GAS, which used not to trigger pre-R6, is
> > irrelevant from this change's point of view and just adds noise.  I'm
> > surprised that it worked even in the first place, as I reckon GCC is
> > supposed to emit regular MIPS code in the `.set nomacro' mode nowadays,
> 
> In fact, GCC works well if IMM is not zero in mips_expand_conditional_trap
> 
>   mode = GET_MODE (XEXP (comparison, 0));
>   op0 = force_reg (mode, op0);
>   if (!(ISA_HAS_COND_TRAPI
>         ? arith_operand (op1, mode)
>         : reg_or_0_operand (op1, mode)))
>     op1 = force_reg (mode, op1);              // <--------- here
> 
> This problem happens due to that GCC trust GAS so much ;)
> It believe that GAS can recognize `TEQ $2,0`.

 Nope, the use of `reg_or_0_operand' (and the `J' constraint) implies the 
use of the `z' print operand modifier in the output template, there's no 
immediate operand expected to be ever produced from the output template in 
this case, which is the very bug (from commit 82f84ecbb47c ("MIPS32R6 and 
MIPS64R6 support") BTW) you have fixed.

 It is by pure chance that it worked before, because TEQ is an assembly 
macro (and `.set nomacro' should warn about it and with -Werror ultimately 
prevent assembly from succeeding) rather than a direct machine operation.  
It wouldn't have worked in the latter case at all (i.e. with some other 
instructions; there are existing examples in mips.md).

> >  Overall ISTM there is no need for distinct insns for ISA_HAS_COND_TRAPI
> > and !ISA_HAS_COND_TRAPI cases each and this would better be sorted with
> > predicates and constraints, especially as the output pattern is the same
> > in both cases anyway.  This would prevent special-casing from being needed
> > in `mips_expand_conditional_trap' as well.
> >
> 
> I agree. The patch should be quite simple
> 
>    [(trap_if (match_operator:GPR 0 "trap_comparison_operator"
>                                 [(match_operand:GPR 1 "reg_or_0_operand" "dJ")
>                                  (match_operand:GPR 2 "arith_operand" "dI")])
>             (const_int 0))]
>    "ISA_HAS_COND_TRAPI"
> -  "t%C0\t%z1,%2"
> +  "t%C0\t%z1,%z2"
>    [(set_attr "type" "trap")])

 Nope, this is wrong.

  Maciej

Maciej W. Rozycki <macro@orcam.me.uk> 于2024年6月28日周五 01:01写道：
>
> On Thu, 27 Jun 2024, YunQiang Su wrote:
>
> > >  The missed optimisation in GAS, which used not to trigger pre-R6, is
> > > irrelevant from this change's point of view and just adds noise.  I'm
> > > surprised that it worked even in the first place, as I reckon GCC is
> > > supposed to emit regular MIPS code in the `.set nomacro' mode nowadays,
> >
> > In fact, GCC works well if IMM is not zero in mips_expand_conditional_trap
> >
> >   mode = GET_MODE (XEXP (comparison, 0));
> >   op0 = force_reg (mode, op0);
> >   if (!(ISA_HAS_COND_TRAPI
> >         ? arith_operand (op1, mode)
> >         : reg_or_0_operand (op1, mode)))
> >     op1 = force_reg (mode, op1);              // <--------- here
> >
> > This problem happens due to that GCC trust GAS so much ;)
> > It believe that GAS can recognize `TEQ $2,0`.
>
>  Nope, the use of `reg_or_0_operand' (and the `J' constraint) implies the
> use of the `z' print operand modifier in the output template, there's no
> immediate operand expected to be ever produced from the output template in
> this case, which is the very bug (from commit 82f84ecbb47c ("MIPS32R6 and
> MIPS64R6 support") BTW) you have fixed.
>
>  It is by pure chance that it worked before, because TEQ is an assembly
> macro (and `.set nomacro' should warn about it and with -Werror ultimately

In fact it doesn't work.  I find this problem when I tried to fix some
GCC testcases.

> prevent assembly from succeeding) rather than a direct machine operation.
> It wouldn't have worked in the latter case at all (i.e. with some other
> instructions; there are existing examples in mips.md).
>
> > >  Overall ISTM there is no need for distinct insns for ISA_HAS_COND_TRAPI
> > > and !ISA_HAS_COND_TRAPI cases each and this would better be sorted with
> > > predicates and constraints, especially as the output pattern is the same
> > > in both cases anyway.  This would prevent special-casing from being needed
> > > in `mips_expand_conditional_trap' as well.
> > >
> >
> > I agree. The patch should be quite simple
> >
> >    [(trap_if (match_operator:GPR 0 "trap_comparison_operator"
> >                                 [(match_operand:GPR 1 "reg_or_0_operand" "dJ")
> >                                  (match_operand:GPR 2 "arith_operand" "dI")])
> >             (const_int 0))]
> >    "ISA_HAS_COND_TRAPI"
> > -  "t%C0\t%z1,%2"
> > +  "t%C0\t%z1,%z2"
> >    [(set_attr "type" "trap")])
>
>  Nope, this is wrong.
>

> in both cases anyway.  This would prevent special-casing from being needed
> in `mips_expand_conditional_trap' as well.

We cannot make  `mips_expand_conditional_trap' simpler at this point.
As for pre-R6, we have TEQI, so that we can use it if IMM can be
represented with 16bit.
For R6 and IMM out range of 16bit, we have to emit more RTLs/INSNs to
load it into a reg.

Yes, we can merge the two template to

(define_insn "*conditional_trap<mode>"
  [(trap_if (match_operator:GPR 0 "trap_comparison_operator"
                                [(match_operand:GPR 1 "reg_or_0_operand" "dJ")
                                 (match_operand:GPR 2 "arith_operand" "dI")])
            (const_int 0))]
  "ISA_HAS_COND_TRAP"
  {
     if (!ISA_HAS_COND_TRAPI && !reg_or_0_operand(operands[2],
GET_MODE(operands[2])))
        gcc_unreachable();
     return "t%C0\t%z1,%z2";
   }
  [(set_attr "type" "trap")])


>   Maciej

On Fri, 28 Jun 2024, YunQiang Su wrote:

> > > >  Overall ISTM there is no need for distinct insns for ISA_HAS_COND_TRAPI
> > > > and !ISA_HAS_COND_TRAPI cases each and this would better be sorted with
> > > > predicates and constraints, especially as the output pattern is the same
> > > > in both cases anyway.  This would prevent special-casing from being needed
> > > > in `mips_expand_conditional_trap' as well.
> > > >
> > >
> > > I agree. The patch should be quite simple
> > >
> > >    [(trap_if (match_operator:GPR 0 "trap_comparison_operator"
> > >                                 [(match_operand:GPR 1 "reg_or_0_operand" "dJ")
> > >                                  (match_operand:GPR 2 "arith_operand" "dI")])
> > >             (const_int 0))]
> > >    "ISA_HAS_COND_TRAPI"
> > > -  "t%C0\t%z1,%2"
> > > +  "t%C0\t%z1,%z2"
> > >    [(set_attr "type" "trap")])
> >
> >  Nope, this is wrong.
> >
> 
> > in both cases anyway.  This would prevent special-casing from being needed
> > in `mips_expand_conditional_trap' as well.
> 
> We cannot make  `mips_expand_conditional_trap' simpler at this point.

 This is simply not true.  However as the platform maintainer you are the 
expert in this area, so I am leaving it to up you to figure out.  If you 
want, that is, of course.  All the necessary details are in the paragraph 
I've left quoted at the top.

 NB given that this is a fix for an easily reproducible bug, there should 
have been a test case committed along with it.

  Maciej