On Thu, Oct 27, 2016 at 08:44:02PM +0000, Michael Collison wrote:
> This patch is designed to improve code generation for "and" instructions with
> certain immediate operands.
>
> For the following test case:
>
> int f2(int x)
> {
> x &= 0x0ffffff8;
>
> x &= 0xff001fff;
>
> return x;
> }
>
> the trunk aarch64 compiler generates:
>
> mov w1, 8184
> movk w1, 0xf00, lsl 16
> and w0, w0, w1
>
> We can generate one fewer instruction if we recognize certain constants. With
> the attached patch the current trunk compiler generates:
>
> and w0, w0, 268435448
> and w0, w0, -16769025
>
> Bootstrapped and make check successfully completed with no regressions on
> aarch64-linux-gnu.
>
> Okay for trunk?
Hi Michael,
I have some minor comments in line.
> diff --git a/gcc/config/aarch64/aarch64-protos.h
> b/gcc/config/aarch64/aarch64-protos.h
> index 3cdd69b..7ef8cdf 100644
> --- a/gcc/config/aarch64/aarch64-protos.h
> +++ b/gcc/config/aarch64/aarch64-protos.h
> @@ -300,6 +300,9 @@ extern struct tune_params aarch64_tune_params;
> HOST_WIDE_INT aarch64_initial_elimination_offset (unsigned, unsigned);
> int aarch64_get_condition_code (rtx);
> bool aarch64_bitmask_imm (HOST_WIDE_INT val, machine_mode);
> +unsigned HOST_WIDE_INT aarch64_and_split_imm1 (HOST_WIDE_INT val_in);
> +unsigned HOST_WIDE_INT aarch64_and_split_imm2 (HOST_WIDE_INT val_in);
> +bool aarch64_and_bitmask_imm (unsigned HOST_WIDE_INT val_in, machine_mode
> mode);
> int aarch64_branch_cost (bool, bool);
> enum aarch64_symbol_type aarch64_classify_symbolic_expression (rtx);
> bool aarch64_const_vec_all_same_int_p (rtx, HOST_WIDE_INT);
> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> index 3e663eb..db82c5c 100644
> --- a/gcc/config/aarch64/aarch64.c
> +++ b/gcc/config/aarch64/aarch64.c
> @@ -3600,6 +3600,46 @@ aarch64_bitmask_imm (HOST_WIDE_INT val_in,
> machine_mode mode)
> return val == mask * bitmask_imm_mul[__builtin_clz (bits) - 26];
> }
>
> +/* Create mask of ones, covering the lowest to highest bits set in VAL_IN.
> */
> +
> +unsigned HOST_WIDE_INT
> +aarch64_and_split_imm1 (HOST_WIDE_INT val_in)
> +{
> + int first_bit_set = ctz_hwi (val_in);
> + int last_bit_set = floor_log2 (val_in);
> + gcc_assert (first_bit_set < last_bit_set);
This assert can never trigger (by definition) unless VAL_IN == 0 (in which
case floor_log2 = -1, ctz_hwi = HOST_BITS_PER_WORD_SIZE). If that's the case
you're testing for, then this assert would be more explicit as
gcc_assert (val_in != 0)
I'd suggest renaming these to lowest_bit_set and highest_bit_set, as first
and last are ambiguous (you have them the opposite way round from what
I'd consider first [highest, thus leading bits] and last
[lowest/trailing bits]).
> +
> + return ((HOST_WIDE_INT_UC (2) << last_bit_set) -
> + (HOST_WIDE_INT_1U << first_bit_set));
> +}
> +
> +/* Create constant with zero bits between first and last bit set and one
> + bits elsewhere. */
I can't parse this comment in relation to what the code below does.
Looking at the code, you're building a new constant where the bits between
the first and last bits are unmodified, and all other bits are set to one.
i.e. for 0000 1010 1000 you'd generate 1111 1010 1111.
> +unsigned HOST_WIDE_INT
> +aarch64_and_split_imm2 (HOST_WIDE_INT val_in)
> +{
> + return val_in | ~aarch64_and_split_imm1 (val_in);
> +}
> +
> +/* Return true if VAL_IN is a valid 'and' bitmask immediate. */
> +
> +bool
> +aarch64_and_bitmask_imm (unsigned HOST_WIDE_INT val_in, machine_mode mode)
> +{
> + if (aarch64_bitmask_imm (val_in, mode))
> + return false;
> +
> + if (aarch64_move_imm (val_in, mode))
> + return false;
> +
> + unsigned HOST_WIDE_INT imm2 = aarch64_and_split_imm2 (val_in);
> +
> + if (!aarch64_bitmask_imm (imm2, mode))
> + return false;
> +
> + return true;
You can replace these four lines with:
return aarch64_bitmask_imm (imm2, mode);
> +}
>
> /* Return true if val is an immediate that can be loaded into a
> register in a single instruction. */
> diff --git a/gcc/testsuite/gcc.target/aarch64/and_const.c
> b/gcc/testsuite/gcc.target/aarch64/and_const.c
> new file mode 100644
> index 0000000..9c377d7
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/and_const.c
> @@ -0,0 +1,14 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2" } */
> +
> +int f2 (int x)
> +{
> + x &= 0x0ffffff8;
> +
> + x &= 0xff001fff;
> +
> + return x;
> +}
It would be good to have a test for the DImode cases too (using long long).
Thanks,
James
