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
