Re: [PATCH, MIPS] Add seq Octeon instruction

[Richard Sandiford <rdsandiford at googlemail dot com>]

Adam Nemet <anemet@caviumnetworks.com> writes:
> 	* config/mips/mips.h (ISA_HAS_SEQ_SNE): New macro.
> 	* config/mips/mips.c (mips_expand_scc): Also expand seq and sne if
> 	second operand is a reg_imm10_operand.
> 	* config/mips/mips.md (*seq_<GPR:mode><GPR2:mode>_seq,
> 	*sne_<GPR:mode><GPR2:mode>_sne): New patterns.
> 	(*seq_<GPR:mode><GPR2:mode>): Rename to
> 	*seq_zero_<GPR:mode><GPR2:mode>.
> 	(*seq_<GPR:mode><GPR2:mode>_mips16): Rename to
> 	*seq_zero_<GPR:mode><GPR2:mode>_mip16.
> 	(*sne_<GPR:mode><GPR2:mode>): Rename to
> 	*sne_zero_<GPR:mode><GPR2:mode>.
>
> testsuite/
> 	* gcc.target/mips/seq-1.c: New test.
> 	* gcc.target/mips/octeon-seq-1.c: New test.
> 	* gcc.target/mips/octeon-seq-2.c: New test.
> 	* gcc.target/mips/octeon-seq-3.c: New test.
> 	* gcc.target/mips/octeon-seq-4.c: New test.

I'm OK with this, but it's generally better to have a single
matching pattern where possible.  Insns aren't necessarily
rematched after reload, so it is technically possible for
the new pattern to be used for zero comparisons.  E.g. we
might foolishly have forced zero into a register, and then
been unable to allocate that register.  Reload will then
rewrite a two-register seq as an seq against zero.

(An unlikely sequence of events for sure, especially since
insns are going to be rematched when optimisation is enabled.
But You Never Know.)

So if we want to use sltiu for zero comparisons, I think
it would be technically better to have something like:

(define_insn "*seq_<GPR:mode><GPR2:mode>_seq"
  [(set (match_operand:GPR2 0 "register_operand" "=d,d,d")
	(eq:GPR2 (match_operand:GPR 1 "register_operand" "%d,d,d")
		 (match_operand:GPR 2 "reg_imm10_operand" "d,J,YB")))]
  "ISA_HAS_SEQ_SNE"
  "@
    seq\t%0,%1,%2
    sltiu\t%0,%1,1
    seqi\t%0,%1,%2"
  [(set_attr "type" "slt")
   (set_attr "mode" "<GPR:MODE>")])

and add !ISA_HAS_SEQ_SNE to the two zero patterns (both MIPS16 and
non-MIPS16).

BTW, I'd missed the lack of "!TARGET_MIPS16" in the new ISA_HAS_* macros.
Sorry about that.  Could you add them at some point?  (Doesn't matter
whether it's part of another patch or separate.)  Preapproved, and doesn't
need a full retest.

(I know it's daft testing !TARGET_MIPS16 on a processor that doesn't
support MIPS16, but still, it is theoretically a supported combination.)

Richard