On Thu, 2020-06-18 at 17:01 -0500, Bill Schmidt wrote:
> Thanks for the review, Will!  Responses below...
> 
> On 6/18/20 11:08 AM, will schmidt wrote:
> > On Wed, 2020-06-17 at 14:46 -0500, Bill Schmidt wrote:
> > > I posted a version of these patches back in stage 4 (February),
> > > but we agreed that holding off until stage 1 was a better idea.
> > > Since then I've made more progress and reorganized the patches
> > > accordingly.  This group of patches lays groundwork, but does not
> > > actually change GCC's behavior yet, other than to generate the
> > > new initialization information and ignore it.
> > > 
> > > The current built-in support in the rs6000 back end requires at
> > > least
> > > a master's degree in spelunking to comprehend.  It's full of
> > > cruft,
> > > redundancy, and unused bits of code, and long overdue for a
> > > replacement.  This is the first part of my project to do that.
> > > 
> > > My intent is to make adding new built-in functions as simple as
> > > adding
> > > a few lines to a couple of files, and automatically generating as
> > > much
> > > of the initialization, overload resolution, and expansion logic
> > > as
> > > possible.  This patch series establishes the format of the input
> > > files
> > > and creates a new program (rs6000-gen-builtins) to:
> > > 
> > >   * Parse the input files into an internal representation;
> > >   * Generate a file of #defines (rs6000-vecdefines.h) for
> > > eventual
> > >     inclusion into altivec.h; and
> > >   * Generate an initialization file to create and initialize
> > > tables of
> > >     built-in functions and overloads.
> > > 
> > > Patches 1, 3-7, and 9-19 contain the logic for rs6000-gen-
> > > builtins.
> > > Patch 8 provides balanced tree search support for parsing
> > > scalability.
> > > Patches 2 and 21-27 provide a first cut at the input files.
> > > Patch 20 incorporates the new code into the GCC build.
> > > Patch 28 adds comments to some existing files that will help
> > > during the transition from the previous builtin mechanism.
> > > 
> > > The patch series is constructed so that any prefix set of the
> > > patches
> > > can be upstreamed without breaking anything, so we can take the
> > > reviews slowly.  There's still plenty of work left, but I think
> > > it
> > > will be helpful to get this big chunk of patches upstream to make
> > > further progress easier.
> > > 
> > > Thanks in advance for your reviews!
> > > 
> > 
> > I've read through the series.  Nothing significant, just a few
> > cosmetic
> > nits, i've called them out below here, versus replying to the
> > individual emails.
> > 
> > generally lgtm.
> > thanks
> > -Will
> > 
> > 
> > > Bill Schmidt (28):
> > >    rs6000: Initial create of rs6000-gen-builtins.c
> > 
> > ok
> > >    rs6000: Add initial input files
> > 
> > Whitespace/tabs in "Legal values of <vectype>" blurb.
> > otherwise ok
> 
> Urk.  Will fix.
> > >    rs6000: Add file support and functions for diagnostic support
> > 
> > ok
> > >    rs6000: Add helper functions for parsing
> > 
> > ok
> > >    rs6000: Add functions for matching types, part 1 of 3
> > 
> > ok
> > 
> > >    rs6000: Add functions for matching types, part 2 of 3
> > 
> > ok
> > 
> > >    rs6000: Add functions for matching types, part 3 of 3
> > 
> > ok
> > 
> > >    rs6000: Red-black tree implementation for balanced tree search
> > 
> > ok
> > 
> > >    rs6000: Main function with stubs for parsing and output
> > 
> > ok
> > 
> > >    rs6000: Parsing built-in input file, part 1 of 3
> > 
> > ok
> > >    rs6000: Parsing built-in input file, part 2 of 3
> > 
> > ok
> > >    rs6000: Parsing built-in input file, part 3 of 3
> > 
> > ok
> > >    rs6000: Parsing of overload input file
> > 
> > use enums or consts instead of hardcoding values ?
> 
> Is this specifically about MAXOVLDSTANZAS, MAXOVLDS, or something
> else?  
> If the former, I guess I can define these in const decls instead of 
> using #define if that's preferred.

No issue with those.  I was noting the constants used as the return
values in the parse_ovld_entry() function. 

You have them clearly documented there.

+/* Parse one two-line entry in the overload file.  Return 0 for EOF, 1 for
+   success, 2 for end-of-stanza, and 6 for a parsing failure.  */

So just a suggestion to use other defined values for that.

I didn't notice those numbers used in the other patches, so maybe this
is already fixed up elsewhere.



Thanks
-Will



Reply via email to