> -----Original Message----- > From: Richard Sandiford <richard.sandif...@arm.com> > Sent: 21 May 2021 13:08 > To: Kyrylo Tkachov <kyrylo.tkac...@arm.com> > Cc: gcc-patches@gcc.gnu.org > Subject: Re: [PATCH] aarch64: Add attributes for builtins specified in > aarch64- > builtins.c > > Kyrylo Tkachov <kyrylo.tkac...@arm.com> writes: > > Hi all, > > > > Besides the builtins in aarch64-simd-builtins.def there are a number of > builtins defined in aarch64-builtins.c itself. > > They could also benefit from the attributes generated by > aarch64_get_attributes. > > However aarch64_get_attributes and its helpers are only set up to handle a > aarch64_simd_builtin_datum. > > > > This patch changes these functions to instead take a flag and mode value > that are extracted from > > aarch64_simd_builtin_datum.flags and aarch64_simd_builtin_datum.mode > anyway. > > Then the various builtin init functions in aarch64-builtins.c can pass down > their own FLAG_* flags > > that they want to derive attributes from. > > I'm not too happy with changing the helper functions in this way, but it > seemed like the least bad option. > > > > Richard, does this look like an ok approach to you? > > Looks good to me. Just some very minor things: > > I guess this is personal preference, but in: > > > + unsigned int flags = FLAG_FP; > > + tree attrs = aarch64_get_attributes (flags, d->mode); > > it seemed odd to separate out the flags variable instead of just having: > > tree attrs = aarch64_get_attributes (FLAG_FP, d->mode); > > Long line in: > > > + tree fndecl = aarch64_general_add_builtin (d->name, ftype, d->fcode, > attrs); > > For; > > > @@ -906,14 +906,13 @@ aarch64_init_simd_builtin_scalar_types (void) > > "__builtin_aarch64_simd_udi"); > > } > > > > -/* Return a set of FLAG_* flags that describe what the function could do, > > +/* Adjust the set of FLAG_* flags derived from FLAGS > > + that describe what the function with result MODE could do, > > I think this is easier to parse without the s/Return a/Adjust the/. > > Also, s/the function/a function/ seems better, now that we're not passing > a specific function. Same for the other comments.
Thanks for reviewing. I've pushed the fixed patch to trunk. Kyrill > > Thanks, > Richard
attrs.patch
Description: attrs.patch
