On Mon, Nov 09, 2015 at 11:03:28AM +0000, Bilyan Borisov wrote:
>
>
> On 03/11/15 11:16, James Greenhalgh wrote:
> >On Fri, Oct 30, 2015 at 09:31:08AM +0000, Bilyan Borisov wrote:
> >>In this patch from the series, all vmulx_lane variants have been
> >>implemented as
> >>a vdup followed by a vmulx. Existing implementations of intrinsics were
> >>refactored to use this new approach.
> >>
> >>Several new nameless md patterns are added that will enable the combine
> >>pass to
> >>pick up the dup/fmulx combination and replace it with a proper fmulx[lane]
> >>instruction.
> >>
> >>In addition, test cases for all new intrinsics were added. Tested on targets
> >>aarch64-none-elf and aarch64_be-none-elf.
> >Hi,
> >
> >I have a small style comment below.
> >
> >>gcc/
> >>
> >>2015-XX-XX Bilyan Borisov <bilyan.bori...@arm.com>
> >>
> >> * config/aarch64/arm_neon.h (vmulx_lane_f32): New.
> >> (vmulx_lane_f64): New.
> >> (vmulxq_lane_f32): Refactored & moved.
> >> (vmulxq_lane_f64): Refactored & moved.
> >> (vmulx_laneq_f32): New.
> >> (vmulx_laneq_f64): New.
> >> (vmulxq_laneq_f32): New.
> >> (vmulxq_laneq_f64): New.
> >> (vmulxs_lane_f32): New.
> >> (vmulxs_laneq_f32): New.
> >> (vmulxd_lane_f64): New.
> >> (vmulxd_laneq_f64): New.
> >> * config/aarch64/aarch64-simd.md (*aarch64_combine_dupfmulx1<mode>,
> >> VDQSF): New pattern.
> >> (*aarch64_combine_dupfmulx2<mode>, VDQF): New pattern.
> >> (*aarch64_combine_dupfmulx3): New pattern.
> >> (*aarch64_combine_vgetfmulx1<mode>, VDQF_DF): New pattern.
> >I'm not sure I like the use of 1,2,3 for this naming scheme. Elsewhere in
> >the file, this convention points to the number of operands a pattern
> >requires (for example add<mode>3).
> >
> >I think elsewhere in the file we use:
> >
> >
> > "*aarch64_mul3_elt<mode>"
> > "*aarch64_mul3_elt_<vswap_width_name><mode>"
> > "*aarch64_mul3_elt_to_128df"
> > "*aarch64_mul3_elt_to_64v2df"
> >
> >Is there a reason not to follow that pattern?
> >
> >Thanks,
> >James
> >
> Hi,
>
> I've made the changes you've requested - the pattern names have been
> changed to follow better the naming conventions elsewhere in the
> file.
This is OK with a reformatting of some comments.
> +;; fmulxs_lane_f32, fmulxs_laneq_f32, fmulxd_lane_f64 == fmulx_lane_f64,
> +;; fmulxd_laneq_f64 == fmulx_laneq_f64
I'd rewrite this as so:
;; fmulxs_lane_f32, fmulxs_laneq_f32
;; fmulxd_lane_f64 == fmulx_lane_f64
;; fmulxd_laneq_f64 == fmulx_laneq_f64
The way you have it I was parsing it as all of {fmulxs_lane_f32,
fmulxs_laneq_f32, fmulxd_lane_f64} are the same as fmulx_lane_f64 - which
is not accurate.
Additionally, with all these comments I'd use the intrinsic name
(vmulx_lane_f32 rather than fmulx_lane_f32).
Sorry for the long wait for review.
I've committed it on your behalf as revision r230720.
Thanks,
James
