RE: [PATCH 5/5][AArch64] fp16fml support

2018-01-10 Thread Michael Collison 
Okay will put on my to-do list for post GCC 8.

-Original Message-
From: James Greenhalgh [mailto:james.greenha...@arm.com] 
Sent: Wednesday, January 10, 2018 12:21 PM
To: Michael Collison <michael.colli...@arm.com>
Cc: Richard Sandiford <richard.sandif...@linaro.org>; GCC Patches 
<gcc-patches@gcc.gnu.org>; nd <n...@arm.com>
Subject: Re: [PATCH 5/5][AArch64] fp16fml support

On Tue, Jan 09, 2018 at 06:28:09PM +, Michael Collison wrote:
> Patch updated per Richard's comments. Ok for trunk?

This patch adds a lot of code, much of which looks like it ought to be possible 
to common up using the iterators. I'm going to OK it as is, as I'd like to see 
this make GCC 8, and we've sat on it for long enough, but I would really 
appreciate futurec refactoring in this area. I'm worried about maintainability 
as it stands.

OK.

Thanks,
James

> 
> -Original Message-
> From: Richard Sandiford [mailto:richard.sandif...@linaro.org]
> Sent: Thursday, January 4, 2018 8:02 AM
> To: Michael Collison <michael.colli...@arm.com>
> Cc: GCC Patches <gcc-patches@gcc.gnu.org>; nd <n...@arm.com>
> Subject: Re: [PATCH 5/5][AArch64] fp16fml support
> 
> Hi Michael,
> 
> Not a review of the full patch, just a comment about the patterns:
> 
> Michael Collison <michael.colli...@arm.com> writes:
> > +(define_expand "aarch64_fmll_lane_lowv2sf"
> > +  [(set (match_operand:V2SF 0 "register_operand" "")
> > +   (unspec:V2SF [(match_operand:V2SF 1 "register_operand" "")
> > +  (match_operand:V4HF 2 "register_operand" "")
> > +  (match_operand:V4HF 3 "register_operand" "")
> > +  (match_operand:SI 4 "aarch64_imm2" "")]
> > +VFMLA16_LOW))]
> > +  "TARGET_F16FML"
> > +{
> > +rtx p1 = aarch64_simd_vect_par_cnst_half (V4HFmode,
> > + GET_MODE_NUNITS (V4HFmode),
> > + false);
> > +rtx lane = GEN_INT (ENDIAN_LANE_N (GET_MODE_NUNITS (SImode), 
> > +INTVAL (operands[4])));
> 
> Please use the newly-introduced aarch64_endian_lane_rtx for this.
> 
> GET_MODE_NUNITS (SImode) doesn't seem right though, since that's always 1.
> Should it be using V4HFmode instead?
> 
> Same for the other patterns.
> 
> Thanks,
> Richard

Re: [PATCH 5/5][AArch64] fp16fml support

2018-01-10 Thread James Greenhalgh 
On Tue, Jan 09, 2018 at 06:28:09PM +, Michael Collison wrote:
> Patch updated per Richard's comments. Ok for trunk?

This patch adds a lot of code, much of which looks like it ought to be
possible to common up using the iterators. I'm going to OK it as is, as I'd
like to see this make GCC 8, and we've sat on it for long enough, but I would
really appreciate futurec refactoring in this area. I'm worried about
maintainability as it stands.

OK.

Thanks,
James

> 
> -Original Message-
> From: Richard Sandiford [mailto:richard.sandif...@linaro.org] 
> Sent: Thursday, January 4, 2018 8:02 AM
> To: Michael Collison <michael.colli...@arm.com>
> Cc: GCC Patches <gcc-patches@gcc.gnu.org>; nd <n...@arm.com>
> Subject: Re: [PATCH 5/5][AArch64] fp16fml support
> 
> Hi Michael,
> 
> Not a review of the full patch, just a comment about the patterns:
> 
> Michael Collison <michael.colli...@arm.com> writes:
> > +(define_expand "aarch64_fmll_lane_lowv2sf"
> > +  [(set (match_operand:V2SF 0 "register_operand" "")
> > +   (unspec:V2SF [(match_operand:V2SF 1 "register_operand" "")
> > +  (match_operand:V4HF 2 "register_operand" "")
> > +  (match_operand:V4HF 3 "register_operand" "")
> > +  (match_operand:SI 4 "aarch64_imm2" "")]
> > +VFMLA16_LOW))]
> > +  "TARGET_F16FML"
> > +{
> > +rtx p1 = aarch64_simd_vect_par_cnst_half (V4HFmode,
> > + GET_MODE_NUNITS (V4HFmode),
> > + false);
> > +rtx lane = GEN_INT (ENDIAN_LANE_N (GET_MODE_NUNITS (SImode), INTVAL 
> > (operands[4])));
> 
> Please use the newly-introduced aarch64_endian_lane_rtx for this.
> 
> GET_MODE_NUNITS (SImode) doesn't seem right though, since that's always 1.
> Should it be using V4HFmode instead?
> 
> Same for the other patterns.
> 
> Thanks,
> Richard

RE: [PATCH 5/5][AArch64] fp16fml support

2018-01-09 Thread Michael Collison 
Patch updated per Richard's comments. Ok for trunk?

-Original Message-
From: Richard Sandiford [mailto:richard.sandif...@linaro.org] 
Sent: Thursday, January 4, 2018 8:02 AM
To: Michael Collison <michael.colli...@arm.com>
Cc: GCC Patches <gcc-patches@gcc.gnu.org>; nd <n...@arm.com>
Subject: Re: [PATCH 5/5][AArch64] fp16fml support

Hi Michael,

Not a review of the full patch, just a comment about the patterns:

Michael Collison <michael.colli...@arm.com> writes:
> +(define_expand "aarch64_fmll_lane_lowv2sf"
> +  [(set (match_operand:V2SF 0 "register_operand" "")
> + (unspec:V2SF [(match_operand:V2SF 1 "register_operand" "")
> +(match_operand:V4HF 2 "register_operand" "")
> +(match_operand:V4HF 3 "register_operand" "")
> +(match_operand:SI 4 "aarch64_imm2" "")]
> +  VFMLA16_LOW))]
> +  "TARGET_F16FML"
> +{
> +rtx p1 = aarch64_simd_vect_par_cnst_half (V4HFmode,
> +   GET_MODE_NUNITS (V4HFmode),
> +   false);
> +rtx lane = GEN_INT (ENDIAN_LANE_N (GET_MODE_NUNITS (SImode), INTVAL 
> (operands[4])));

Please use the newly-introduced aarch64_endian_lane_rtx for this.

GET_MODE_NUNITS (SImode) doesn't seem right though, since that's always 1.
Should it be using V4HFmode instead?

Same for the other patterns.

Thanks,
Richard


fp16fml_up_v2.patch
Description: fp16fml_up_v2.patch

Re: [PATCH 5/5][AArch64] fp16fml support

2018-01-04 Thread Richard Sandiford 
Hi Michael,

Not a review of the full patch, just a comment about the patterns:

Michael Collison  writes:
> +(define_expand "aarch64_fmll_lane_lowv2sf"
> +  [(set (match_operand:V2SF 0 "register_operand" "")
> + (unspec:V2SF [(match_operand:V2SF 1 "register_operand" "")
> +(match_operand:V4HF 2 "register_operand" "")
> +(match_operand:V4HF 3 "register_operand" "")
> +(match_operand:SI 4 "aarch64_imm2" "")]
> +  VFMLA16_LOW))]
> +  "TARGET_F16FML"
> +{
> +rtx p1 = aarch64_simd_vect_par_cnst_half (V4HFmode,
> +   GET_MODE_NUNITS (V4HFmode),
> +   false);
> +rtx lane = GEN_INT (ENDIAN_LANE_N (GET_MODE_NUNITS (SImode), INTVAL 
> (operands[4])));

Please use the newly-introduced aarch64_endian_lane_rtx for this.

GET_MODE_NUNITS (SImode) doesn't seem right though, since that's always 1.
Should it be using V4HFmode instead?

Same for the other patterns.

Thanks,
Richard