Hi!
Looks fine to me... Just the same generic things as before, things we
can improve later, not even limited to this series:
On Sat, May 09, 2020 at 08:16:26AM -0500, Bill Schmidt wrote:
> * config/rs6000/altivec.md (UNSPEC_VSTRIR): New constant.
> (UNSPEC_VSTRIL): Likewise.
Names for these could perhaps be better. Or maybe not, they are short
now, there's something to say for that as well :-)
> (vstrir_<mode>): New expansion.
> (vstrir_code_<mode>): New insn.
Could you make this vstrir<mode> and vstrir<mode>_internal, like the
rest?
> (vstrir_p_<mode>): New expansion.
> (vstrir_p_code_<mode>): New insn.
But, not sure what to do with those. "Something to improve later" then
I guess, for all of it :-)
> +(define_expand "vstrir_<mode>"
> + [(set (match_operand:VIshort 0 "altivec_register_operand")
> + (unspec:VIshort [(match_operand:VIshort 1 "altivec_register_operand")]
> + UNSPEC_VSTRIR))]
> + "TARGET_FUTURE"
> +{
> + if (BYTES_BIG_ENDIAN)
> + emit_insn (gen_vstrir_code_<mode> (operands[0], operands[1]));
> + else
> + emit_insn (gen_vstril_code_<mode> (operands[0], operands[1]));
> + DONE;
> +})
So the reason this pattern is special at all is that left and right are
swapped for LE. Maybe that could/should be done in the code for the
builtin, instead?
> +;; This expands into same code as vstrir_<mode> followed by condition logic
> +;; so that a single vstribr. or vstrihr. or vstribl. or vstrihl. instruction
> +;; can, for example, satisfy the needs of a vec_strir () function paired
> +;; with a vec_strir_p () function if both take the same incoming arguments.
> +(define_expand "vstrir_p_<mode>"
> + [(match_operand:SI 0 "gpc_reg_operand")
> + (match_operand:VIshort 1 "altivec_register_operand")]
> + "TARGET_FUTURE"
> +{
> + rtx scratch = gen_reg_rtx (<MODE>mode);
> + if (BYTES_BIG_ENDIAN)
> + emit_insn (gen_vstrir_p_code_<mode> (scratch, operands[1]));
> + else
> + emit_insn (gen_vstril_p_code_<mode> (scratch, operands[1]));
> + emit_insn (gen_cr6_test_for_zero (operands[0]));
> + DONE;
> +})
And the code for the builtin can do this then, as well.
Not sure how easy that is to fit in with the current code, or after your
work on it. Either way, it looks fine to me :-)
Segher
