Hi!
On Mon, Sep 14, 2020 at 05:53:13PM +0800, Kewen.Lin wrote:
> This patch is to extend the existing function find_alignment_op to
> check all defintions of base_reg are AND operations with mask -16B
> to force the alignment. If all are satifised, it passes all AND
> operations and instructions in one vector to recombine_lvx_pattern
> and recombine_stvx_pattern, they will remove all useless ANDs further.
Great :-)
> * config/rs6000/rs6000-p8swap.c (insn_rtx_pair_t): New type.
Please don't do that. The "first" and "second" are completely
meaningless. Also, keeping it separate arrays can very well result in
better machine code, and certainly makes easier to read source code.
> +static bool
> +find_alignment_op (rtx_insn *insn, rtx base_reg,
> + vec<insn_rtx_pair_t> *and_insn_vec)
Don't name vecs "_vec" (just keep it "and_insn" here, or sometimes
and_insns is clearer).
> - rtx and_operation = 0;
> + rtx and_operation = NULL_RTX;
Don't change code randomly (to something arguably worse, even).
> - if (and_operation != 0)
> - break;
> + rtx_insn *and_insn = DF_REF_INSN (base_def_link->ref);
> + and_operation = alignment_mask (and_insn);
> +
> + /* Stop if we find any one which doesn't align. */
> + if (and_operation == NULL_RTX)
> + return false;
No. if (!and_operation) would be fine though :-)
> - return and_operation;
> + return and_operation != NULL_RTX;
It was fine already. You can write !!and_operation if you want to
explicitly convert it to bool (but that is not helpful usually).
> + bool all_and_p = find_alignment_op (insn, base_reg, &and_insn_vec);
This is not a predicate. Do not name it _p please.
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr97019.c
> @@ -0,0 +1,79 @@
> +/* { dg-do compile { target { powerpc_p8vector_ok && le } } } */
Why only on LE? (If there is a reason, the testcase should say; if
there is not, well, it shouldn't say it does then :-) )
Please resend with those things fixed.
Segher
