On Fri, Feb 10, 2023 at 10:59:52AM +0800, Xionghu Luo via Gcc-patches wrote:
So, nothing here is obvious at all still. Could you please split it up
a bit more, so that every step is either small or simple?
So maybe first just split patterns to BE and LE versions, and nothing
else?
And one patch per insn, if at all possible.
This matters so that a regression search will immediately show the
culprit pattern, if anything went wrong.
Most patches will not change anything consequential, but some will, and
it should be very clear which do!
And change (or add) comments in the patch so that I don't have to ask
the same questions as before again! :-)
Most of this seems clean and good, but there is just too much
independent stuff going on at the same time. If your patch series is
split up correctly writing a changelog for it is very easy (this is a
good canary to use!), and if we get regressions from this it should be
trivial to fond the problem, too.
> @@ -3699,13 +3799,13 @@ (define_expand "vec_widen_umult_hi_v16qi"
> {
> emit_insn (gen_altivec_vmuleub (ve, operands[1], operands[2]));
> emit_insn (gen_altivec_vmuloub (vo, operands[1], operands[2]));
> - emit_insn (gen_altivec_vmrghh_direct (operands[0], ve, vo));
> + emit_insn (gen_altivec_vmrghh_direct_be (operands[0], ve, vo));
> }
> else
> {
> emit_insn (gen_altivec_vmuloub (ve, operands[1], operands[2]));
> emit_insn (gen_altivec_vmuleub (vo, operands[1], operands[2]));
> - emit_insn (gen_altivec_vmrghh_direct (operands[0], vo, ve));
> + emit_insn (gen_altivec_vmrghh_direct_le (operands[0], vo, ve));
> }
> DONE;
> })
Please don't. Call the generic gen_vmrg* patterns from the widen
things, don't try to do the compilers job of specialising stuff, it
only makes things much less readable, and causes more mistakes. Just do
like what was there before, essentially.
Segher
