| Message ID | cover.1592419211.git.wschmidt@linux.ibm.com |
|---|---|
| Headers | show |
| Series | rs6000: Auto-generate builtins from descriptions | expand |
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 > 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 ? > rs6000: Build and store function type identifiers ok > rs6000: Write output to the vector definition include file ok > rs6000: Write output to the builtins header file Nit, i'd probably add a 'FIXME' keyword near the "_x" conflict avoidance so this is easy to find later on. That said, this is rather obvious, so nbd. :-) ok. > rs6000: Write output to the builtins init file, part 1 of 3 BILLDEBUG reference should probably be culled. :-) ok > rs6000: Write output to the builtins init file, part 2 of 3 ok > rs6000: Write output to the builtins init file, part 3 of 3 ok > rs6000: Incorporate new builtins code into the build machinery ok > rs6000: Add remaining MASK_ALTIVEC builtins A few very long lines (__builtin_vec_init_v16qi, etc) ; but I think those are fine. ok > rs6000: Add MASK_VSX builtins For the pick-one-or-the-other, i'd tend to the generic looking one. i.e. const vf __builtin_vsx_xvcvuxwsp(vui); CVCVUXWSP_V4SF vsx_xvcvuxwsp {} .. looks better to me. Though there may be subtlety that I don't understand. ok > rs6000: Add available-everywhere and ancient builtins No opinion or thoughts on the packtf or unpacktf entries. ok > rs6000: Add Power7 builtins ok > rs6000: Add MASK_P8_VECTOR builtins ok > rs6000: Add MASK_P9_VECTOR and MASK_P9_MISC builtins ok > rs6000: Add remaining builtins ok > rs6000: Add comments to help with transition Are the deprecations old enough that these can be dropped outright? ok > > gcc/config.gcc | 3 +- > gcc/config/rs6000/rbtree.c | 233 ++ > gcc/config/rs6000/rbtree.h | 51 + > gcc/config/rs6000/rs6000-builtin-new.def | 2965 > ++++++++++++++++++++++ > gcc/config/rs6000/rs6000-builtin.def | 15 + > gcc/config/rs6000/rs6000-call.c | 166 ++ > gcc/config/rs6000/rs6000-gen-builtins.c | 2586 +++++++++++++++++++ > gcc/config/rs6000/rs6000-overload.def | 57 + > gcc/config/rs6000/t-rs6000 | 25 +- > 9 files changed, 6099 insertions(+), 2 deletions(-) > create mode 100644 gcc/config/rs6000/rbtree.c > create mode 100644 gcc/config/rs6000/rbtree.h > create mode 100644 gcc/config/rs6000/rs6000-builtin-new.def > create mode 100644 gcc/config/rs6000/rs6000-gen-builtins.c > create mode 100644 gcc/config/rs6000/rs6000-overload.def >
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. > >> rs6000: Build and store function type identifiers > ok > >> rs6000: Write output to the vector definition include file > ok > >> rs6000: Write output to the builtins header file > Nit, i'd probably add a 'FIXME' keyword near the "_x" conflict > avoidance so this is easy to find later on. That said, this is rather > obvious, so nbd. :-) That's fair, will add. > ok. > >> rs6000: Write output to the builtins init file, part 1 of 3 > BILLDEBUG reference should probably be culled. :-) Thanks, good catch. Needs to be a separate patch on my test branch for the ongoing work... > ok > >> rs6000: Write output to the builtins init file, part 2 of 3 > ok > >> rs6000: Write output to the builtins init file, part 3 of 3 > ok > >> rs6000: Incorporate new builtins code into the build machinery > ok > >> rs6000: Add remaining MASK_ALTIVEC builtins > A few very long lines (__builtin_vec_init_v16qi, etc) ; but I think > those are fine. Yeah, they have to be, with the simplistic parsing strategy I'm using. So long as there aren't many of them, I feel it's not too unreadable. > ok > >> rs6000: Add MASK_VSX builtins > For the pick-one-or-the-other, i'd tend to the generic looking one. > i.e. > const vf __builtin_vsx_xvcvuxwsp(vui); > CVCVUXWSP_V4SF vsx_xvcvuxwsp {} > .. looks better to me. Though there may be subtlety that I don't > understand. Yeah, for now let me add a FIXME to such things. This is just an acknowledgment that currently we have defined two names and patterns where we only need one... > ok > >> rs6000: Add available-everywhere and ancient builtins > No opinion or thoughts on the packtf or unpacktf entries. Me either, yet. :/ As stated, I need to check with SJM whether he has a dependency on this in pveclib, and whether that makes any sense. > ok > >> rs6000: Add Power7 builtins > ok >> rs6000: Add MASK_P8_VECTOR builtins > ok >> rs6000: Add MASK_P9_VECTOR and MASK_P9_MISC builtins > ok > >> rs6000: Add remaining builtins > ok > >> rs6000: Add comments to help with transition > Are the deprecations old enough that these can be dropped outright? They are. I can't destroy them yet until I'm ready to make the complete switchover to the new methods. Thanks again! Bill > > ok > > >> gcc/config.gcc | 3 +- >> gcc/config/rs6000/rbtree.c | 233 ++ >> gcc/config/rs6000/rbtree.h | 51 + >> gcc/config/rs6000/rs6000-builtin-new.def | 2965 >> ++++++++++++++++++++++ >> gcc/config/rs6000/rs6000-builtin.def | 15 + >> gcc/config/rs6000/rs6000-call.c | 166 ++ >> gcc/config/rs6000/rs6000-gen-builtins.c | 2586 +++++++++++++++++++ >> gcc/config/rs6000/rs6000-overload.def | 57 + >> gcc/config/rs6000/t-rs6000 | 25 +- >> 9 files changed, 6099 insertions(+), 2 deletions(-) >> create mode 100644 gcc/config/rs6000/rbtree.c >> create mode 100644 gcc/config/rs6000/rbtree.h >> create mode 100644 gcc/config/rs6000/rs6000-builtin-new.def >> create mode 100644 gcc/config/rs6000/rs6000-gen-builtins.c >> create mode 100644 gcc/config/rs6000/rs6000-overload.def >>
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
On 6/18/20 5:48 PM, will schmidt wrote: > 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. I see, thanks for the explanation. I agree, it's not just in this patch; the return values used in parsing are a mess. I'll create an enum and use it consistently. Thanks! Bill > > > > Thanks > -Will > > >
Gentle ping to keep this in your inbox. :) There are still many more important things ahead of this. Bill On 6/17/20 2:46 PM, Bill Schmidt via Gcc-patches 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! > > > Bill Schmidt (28): > rs6000: Initial create of rs6000-gen-builtins.c > rs6000: Add initial input files > rs6000: Add file support and functions for diagnostic support > rs6000: Add helper functions for parsing > rs6000: Add functions for matching types, part 1 of 3 > rs6000: Add functions for matching types, part 2 of 3 > rs6000: Add functions for matching types, part 3 of 3 > rs6000: Red-black tree implementation for balanced tree search > rs6000: Main function with stubs for parsing and output > rs6000: Parsing built-in input file, part 1 of 3 > rs6000: Parsing built-in input file, part 2 of 3 > rs6000: Parsing built-in input file, part 3 of 3 > rs6000: Parsing of overload input file > rs6000: Build and store function type identifiers > rs6000: Write output to the vector definition include file > rs6000: Write output to the builtins header file > rs6000: Write output to the builtins init file, part 1 of 3 > rs6000: Write output to the builtins init file, part 2 of 3 > rs6000: Write output to the builtins init file, part 3 of 3 > rs6000: Incorporate new builtins code into the build machinery > rs6000: Add remaining MASK_ALTIVEC builtins > rs6000: Add MASK_VSX builtins > rs6000: Add available-everywhere and ancient builtins > rs6000: Add Power7 builtins > rs6000: Add MASK_P8_VECTOR builtins > rs6000: Add MASK_P9_VECTOR and MASK_P9_MISC builtins > rs6000: Add remaining builtins > rs6000: Add comments to help with transition > > gcc/config.gcc | 3 +- > gcc/config/rs6000/rbtree.c | 233 ++ > gcc/config/rs6000/rbtree.h | 51 + > gcc/config/rs6000/rs6000-builtin-new.def | 2965 ++++++++++++++++++++++ > gcc/config/rs6000/rs6000-builtin.def | 15 + > gcc/config/rs6000/rs6000-call.c | 166 ++ > gcc/config/rs6000/rs6000-gen-builtins.c | 2586 +++++++++++++++++++ > gcc/config/rs6000/rs6000-overload.def | 57 + > gcc/config/rs6000/t-rs6000 | 25 +- > 9 files changed, 6099 insertions(+), 2 deletions(-) > create mode 100644 gcc/config/rs6000/rbtree.c > create mode 100644 gcc/config/rs6000/rbtree.h > create mode 100644 gcc/config/rs6000/rs6000-builtin-new.def > create mode 100644 gcc/config/rs6000/rs6000-gen-builtins.c > create mode 100644 gcc/config/rs6000/rs6000-overload.def >