On Mon, Sep 25, 2017 at 11:13:57AM +0100, Sudi Das wrote:
>
> Hi James
>
> I put aarch64_output_simd_general_immediate looking at the similarities of
> the immediates for mov/mvni and orr/bic. The CHECK macro in
> aarch64_simd_valid_immediate both checks
> and converts the immediates in a manner that are needed for the instructions.
>
> Having said that, I agree that maybe I could have refactored
> aarch64_output_simd_mov_immediate to do the work rather than creating a new
> functions to do similar things. I have done so in this patch.
Thanks, this looks much neater.
> I have also changed the names of the enum simd_immediate_check to be better
> indicative of what they are doing.
Thanks, I'd tweak them to look more like the bitmasks you use them as, but
that is a small change for my personal preference.
> Lastly I have added more cases in the tests (according to all the possible
> CHECKs) and made them dg-do assemble (although I had to add --save-temps so
> that the scan-assembler would work). Do you think I should not put that
> option and rather create separate tests?
This is good - thanks.
I think clean up the enum definitions and this patch will be good.
> @@ -308,6 +308,16 @@ enum aarch64_parse_opt_result
> AARCH64_PARSE_INVALID_ARG /* Invalid arch, tune, cpu arg. */
> };
>
> +/* Enum to distinguish which type of check is to be done in
> + aarch64_simd_valid_immediate. This is used as a bitmask where
> + AARCH64_CHECK_MOV has both bits set. Thus AARCH64_CHECK_MOV will
> + perform all checks. Adding new types would require changes accordingly.
> */
> +enum simd_immediate_check {
> + AARCH64_CHECK_ORR = 1, /* Perform immediate checks for ORR. */
> + AARCH64_CHECK_BIC = 2, /* Perform immediate checks for BIC. */
> + AARCH64_CHECK_MOV = 3 /* Perform all checks (used for MOVI/MNVI). */
These are used in bit-mask style, so how about:
AARCH64_CHECK_ORR = 1 << 0,
AARCH64_CHECK_BIC = 1 << 1,
AARCH64_CHECK_MOV = AARCH64_CHECK_ORR | AARCH64_CHECK_BIC
Which is more self-documenting.
> @@ -13001,7 +13013,8 @@ aarch64_float_const_representable_p (rtx x)
> char*
> aarch64_output_simd_mov_immediate (rtx const_vector,
> machine_mode mode,
> - unsigned width)
> + unsigned width,
> + enum simd_immediate_check which)
This function is sorely missing a comment explaining the parameters - it
would be very helpful if you could add one as part of this patch.
Thanks,
James
