This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
RE: [PATCH] MIPS: Fix mode mismatch error between Loongson builtin arguments and insn operands.
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