| Message ID | 5575615E.30509@arm.com |
|---|---|
| State | New |
| Headers | show |
On 8 June 2015 at 10:33, Alan Lawrence <alan.lawrence@arm.com> wrote: > Thanks for working on this! > > I'd been fiddling around with a patch with some similar elements to this, > but many trials with union types, subregs, etc., all worsened the register > allocation and led to more unnecessary shuffling / moves. Kugan has been looking into this at Linaro. We should avoid duplicating effort here. > The only real > thing I tried which you don't do here, was to introduce a set_dreg expander > to clean up some of those macro definitions in arm_neon.h. That could easily > follow in a separate patch if desired! I'd prefer that to be a separate step. > So your patch looks good to me. > > A couple of style nits: > > --- a/gcc/config/aarch64/aarch64-builtins.c > +++ b/gcc/config/aarch64/aarch64-builtins.c > @@ -128,7 +128,9 @@ enum aarch64_type_qualifiers > /* Polynomial types. */ > qualifier_poly = 0x100, > /* Lane indices - must be in range, and flipped for bigendian. */ > - qualifier_lane_index = 0x200 > + qualifier_lane_index = 0x200, > + /* Lane indices for single lane structure loads and stores */ > + qualifier_struct_load_store_lane_index = 0x400 > }; > > should be ...'loads and stores. */' > > also the dg-error messages in the testsuite, do not need to be on the same > line as the statement generating the error, because the trailing 0 tells dg > that the position/line number doesn't matter (i.e. dg should allow the error > to be reported at any line); so these could be brought under 80 chars. OK, thanks. I'll re-spin once I've tested on big endian. > Oh, have you tested bigendian? I have started a bigendian build on our validation infrastructure here. Thanks for the review Charles
Charles Baylis wrote: > On 8 June 2015 at 10:33, Alan Lawrence <alan.lawrence@arm.com> wrote: >> Thanks for working on this! >> >> I'd been fiddling around with a patch with some similar elements to this, >> but many trials with union types, subregs, etc., all worsened the register >> allocation and led to more unnecessary shuffling / moves. > > Kugan has been looking into this at Linaro. We should avoid > duplicating effort here. Yes. I stopped short of looking into the internals of the register allocator, although I believe any proper solution is going to have to make changes here. However, I am working on (/nearly finished, just some tidying!) a patch series to add D-registers to TARGET_ARRAY_MODE_SUPPORTED_P, which may help matters. >> The only real >> thing I tried which you don't do here, was to introduce a set_dreg expander >> to clean up some of those macro definitions in arm_neon.h. That could easily >> follow in a separate patch if desired! > > I'd prefer that to be a separate step. Sure. (*If* we go that route - I hope to have another look after aarch64_array_mode_supported_p). Cheers, Alan
--- a/gcc/config/aarch64/aarch64-builtins.c +++ b/gcc/config/aarch64/aarch64-builtins.c @@ -128,7 +128,9 @@ enum aarch64_type_qualifiers /* Polynomial types. */ qualifier_poly = 0x100, /* Lane indices - must be in range, and flipped for bigendian. */ - qualifier_lane_index = 0x200 + qualifier_lane_index = 0x200, + /* Lane indices for single lane structure loads and stores */ + qualifier_struct_load_store_lane_index = 0x400 }; should be ...'loads and stores. */'