On 27/09/17 18:57, Sudi Das wrote:
>
>
> Hi James
>
> I have made the requested changes to the patch.
>
>
> 2017-09-27 Sudakshina Das <sudi....@arm.com>
>
> * config/aarch64/aarch64-protos.h (enum simd_immediate_check): New
> check type
> for aarch64_simd_valid_immediate.
> (aarch64_output_simd_mov_immediate): Update prototype.
> (aarch64_simd_valid_immediate): Update prototype.
>
> * config/aarch64/aarch64-simd.md (orr<mode>3): modified pattern to add
> support for ORR-immediate.
> (and<mode>3): modified pattern to add support for BIC-immediate.
>
> * config/aarch64/aarch64.c (aarch64_simd_valid_immediate): Function now
> checks
> for valid immediate for BIC and ORR based on new enum argument.
> (aarch64_output_simd_mov_immediate): Function now used to output
> BIC/ORR imm
> as well based on new enum argument.
>
> * config/aarch64/constraints.md (Do): New vector immediate constraint.
> (Db): Likewise.
>
> 2017-09-27 Sudakshina Das <sudi....@arm.com>
>
> * gcc.target/aarch64/bic_imm_1.c: New test.
> * gcc.target/aarch64/orr_imm_1.c: Likewise.
>
>
> Thanks
> Sudi
>
>
> From: James Greenhalgh <james.greenha...@arm.com>
> Sent: Tuesday, September 26, 2017 8:04:38 PM
> To: Sudi Das
> Cc: Richard Earnshaw; gcc-patches@gcc.gnu.org; nd; Marcus Shawcroft
> Subject: Re: [PATCH][AArch64] Add BIC-imm and ORR-imm SIMD pattern
>
> 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
>
>
>
+;; For AND (vector, register) and BIC (vector, immediate)
(define_insn "and<mode>3"
- [(set (match_operand:VDQ_I 0 "register_operand" "=w")
- (and:VDQ_I (match_operand:VDQ_I 1 "register_operand" "w")
- (match_operand:VDQ_I 2 "register_operand" "w")))]
+ [(set (match_operand:VDQ_I 0 "register_operand" "=w,w")
+ (and:VDQ_I (match_operand:VDQ_I 1 "register_operand" "w,0")
+ (match_operand:VDQ_I 2 "nonmemory_operand" "w,Db")))]
You should define a new predicate operation for operand 2 that accepts
just registers or the valid constants. Otherwise you'll may get
spilling during register allocation.
Similarly for the other pattern.
R.
