| Message ID | 20210604072909.575524-1-claziss@synopsys.com |
|---|---|
| State | New |
| Headers | show |
| Series | arc: Add --with-fpu support for ARCv2 cpus | expand |
On Fri, 4 Jun 2021 10:29:09 +0300 Claudiu Zissulescu via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > Hi Jeff, > > I would like to add spport for selecting the ARCv2 FPU extension at > configuration-time. > > The --with-fpu configuration option is ignored when -mfpu compiler > option is specified. > > My concern is using `grep -P` when configuring. Is that ok? Please don't. Not every grep(1) has PCRE support so it would be great if you could rephrase it to use just use normal regexp or ERE. like e.g.: grep -q -E "^ARC_CPU \(hs38,[ ]*[emhs]+," config/arc/arc-cpus.def where the space is <space><^vi>, i.e. space, tab Or use an awk script that exits appropriately if the arg matches ARCH. TIA,
On 6/4/2021 1:29 AM, Claudiu Zissulescu via Gcc-patches wrote: > Hi Jeff, > > I would like to add spport for selecting the ARCv2 FPU extension at > configuration-time. > > The --with-fpu configuration option is ignored when -mfpu compiler > option is specified. > > My concern is using `grep -P` when configuring. Is that ok? > > Thanks, > Claudiu > > gcc/ > yyyy-mm-dd Claudiu Zissulescu <claziss@synopsys.com> > > * config.gcc (arc): Add support for with_cpu option. > * config/arc/arc.h (OPTION_DEFAULT_SPECS): Add fpu. I strongly suspect -P is a GNU-ism and probably won't work on other hosts were GCC is still used. I'd avoid it and look for an alternate solution. jeff
Thank you for your input. I have made an update using grep's ERE. Please let me know if it is ok. //Claudiu
On Tue, 8 Jun 2021 10:05:28 +0300 Claudiu Zissulescu <claziss@gmail.com> wrote: > Thank you for your input. > > I have made an update using grep's ERE. Please let me know if it is ok. I would have written [[:space:]]* instead of [[:space:]]+ to handle potentially missing space, at least after the comma but also before the comma to avoid surprises for new names in the future. Furthermore <space>|<tab> alone would be [[:blank:]]* but as you prefer. grep ... > /dev/null would be grep -q which is mandated by POSIX since at least SUSv2 so can be used safely since quite some time now. Instead of the redundant 'true' calls, i'd usually write : E.g. if grep -q ... ; then : else echo "nah"; exit 1 fi Which could be shortened to if ! grep -q ... then echo "nah" exit 1 fi to avoid any questions about an empty arm in the first place. ISTM you only set the expected flags in the switch so i would have set only that variable and have grepped only once after the switch for brevity. Either way, thanks for not using grep -P :) thanks,
Hi, > I would have written [[:space:]]* instead of [[:space:]]+ to handle > potentially missing space, at least after the comma but also before the > comma to avoid surprises for new names in the future. > Furthermore <space>|<tab> alone would be [[:blank:]]* but as you prefer. > > grep ... > /dev/null would be grep -q which is mandated by POSIX since > at least SUSv2 so can be used safely since quite some time now. > > Instead of the redundant 'true' calls, i'd usually write : > E.g. > if grep -q ... ; then : > else echo "nah"; exit 1 > fi > > Which could be shortened to > if ! grep -q ... > then > echo "nah" > exit 1 > fi > > to avoid any questions about an empty arm in the first place. I've updated the patch using your feedback (attached). Indeed, it looks much better now :) > > ISTM you only set the expected flags in the switch so i would have > set only that variable and have grepped only once after the switch for > brevity. ARC has various FPU extensions, some of them are common to EM and HS architectures, others are specific for only one of them. Hence, the grep commands are ensuring that we accept the right fpu extension for the right ARC architecture. > > Either way, thanks for not using grep -P :) > thanks, > I thank you! Claudiu
On Wed, 9 Jun 2021 14:35:01 +0300 Claudiu Zissulescu <claziss@gmail.com> wrote: > > ISTM you only set the expected flags in the switch so i would have > > set only that variable and have grepped only once after the switch for > > brevity. > > ARC has various FPU extensions, some of them are common to EM and HS > architectures, others are specific for only one of them. Hence, the grep > commands are ensuring that we accept the right fpu extension for the > right ARC architecture. Right. Which you'd accomplish more terse if you would set flags_ok in the switch cited below and after the switch have one single grep à la if [ -n "$flags_ok" ] \ && ! grep -q -E "^ARC_CPU[[:blank:]]*\($new_cpu,[[:blank:]]*$flags_ok," \ ${srcdir}/config/arc/arc-cpus.def then echo "Unknown floating point type used in "\ "--with-fpu=$with_fpu for cpu $new_cpu" 1>&2 exit 1 fi The reason is that all case statements in the $with_fpu switch just differ in the allowed flags AFAICS. But as you prefer. last comments below. > diff --git a/gcc/config.gcc b/gcc/config.gcc > index 13c2004e3c52..09886c8635e0 100644 > --- a/gcc/config.gcc > +++ b/gcc/config.gcc > @@ -4258,18 +4258,61 @@ case "${target}" in > ;; > > arc*-*-*) > - supported_defaults="cpu" > + supported_defaults="cpu fpu" > > + new_cpu=hs38_linux > if [ x"$with_cpu" = x ] \ > - || grep "^ARC_CPU ($with_cpu," \ > + || grep -E "^ARC_CPU \($with_cpu," \ > ${srcdir}/config/arc/arc-cpus.def \ > > /dev/null; then Cosmetics: You may want to keep the non "-E" version but use -q and drop the redirect to /dev/null. > # Ok > - true > + new_cpu=$with_cpu > else > echo "Unknown cpu used in --with-cpu=$with_cpu" 1>&2 > exit 1 > fi > + > + # see if --with-fpu matches any of the supported FPUs > + case "$with_fpu" in > + "") > + # OK > + ;; > + fpus | fpus_div | fpus_fma | fpus_all) > + # OK if em or hs > + if ! grep -q -E "^ARC_CPU[[:blank:]]*\($new_cpu,[[:space:]]*[emhs]+," \ you changed only the first :space: to :blank: (everywhere) > + ${srcdir}/config/arc/arc-cpus.def > + then > + echo "Unknown floating point type used in "\ > + "--with-fpu=$with_fpu for cpu $new_cpu" 1>&2 > + exit 1 > + fi > + ;; > + fpuda | fpuda_div | fpuda_fma | fpuda_all) > + # OK only em > + if ! grep -q -E "^ARC_CPU[[:blank:]]*\($new_cpu,[[:space:]]*em," \ > + ${srcdir}/config/arc/arc-cpus.def > + then > + echo "Unknown floating point type used in "\ > + "--with-fpu=$with_fpu for cpu $new_cpu" 1>&2 > + exit 1 > + fi > + ;; > + fpud | fpud_div | fpud_fma | fpud_all) > + # OK only hs > + if ! grep -q -E "^ARC_CPU[[:blank:]]*\($new_cpu,[[:space:]]*hs," \ > + ${srcdir}/config/arc/arc-cpus.def > + then > + echo "Unknown floating point type used in"\ > + "--with-fpu=$with_fpu for cpu $new_cpu" 1>&2 missing trailing space after 'in' in "used in"\ LGTM with that fixed with or without cutting down on grep lines, but of course i cannot approve it. thanks, > + exit 1 > + fi > + ;; > + *) > + echo "Unknown floating point type used in "\ > + "--with-fpu=$with_fpu" 1>&2 > + exit 1 > + ;; > + esac > ;; > > csky-*-*)
On 6/9/2021 6:26 AM, Bernhard Reutner-Fischer wrote: > On Wed, 9 Jun 2021 14:35:01 +0300 > Claudiu Zissulescu <claziss@gmail.com> wrote: > >>> ISTM you only set the expected flags in the switch so i would have >>> set only that variable and have grepped only once after the switch for >>> brevity. >> ARC has various FPU extensions, some of them are common to EM and HS >> architectures, others are specific for only one of them. Hence, the grep >> commands are ensuring that we accept the right fpu extension for the >> right ARC architecture. > Right. Which you'd accomplish more terse if you would set flags_ok in > the switch cited below and after the switch have one single grep à la > > if [ -n "$flags_ok" ] \ > && ! grep -q -E "^ARC_CPU[[:blank:]]*\($new_cpu,[[:blank:]]*$flags_ok," \ > ${srcdir}/config/arc/arc-cpus.def > then > echo "Unknown floating point type used in "\ > "--with-fpu=$with_fpu for cpu $new_cpu" 1>&2 > exit 1 > fi > > The reason is that all case statements in the $with_fpu switch just > differ in the allowed flags AFAICS. But as you prefer. > > last comments below. > >> diff --git a/gcc/config.gcc b/gcc/config.gcc >> index 13c2004e3c52..09886c8635e0 100644 >> --- a/gcc/config.gcc >> +++ b/gcc/config.gcc >> @@ -4258,18 +4258,61 @@ case "${target}" in >> ;; >> >> arc*-*-*) >> - supported_defaults="cpu" >> + supported_defaults="cpu fpu" >> >> + new_cpu=hs38_linux >> if [ x"$with_cpu" = x ] \ >> - || grep "^ARC_CPU ($with_cpu," \ >> + || grep -E "^ARC_CPU \($with_cpu," \ >> ${srcdir}/config/arc/arc-cpus.def \ >> > /dev/null; then > Cosmetics: You may want to keep the non "-E" version but use -q > and drop the redirect to /dev/null. > >> # Ok >> - true >> + new_cpu=$with_cpu >> else >> echo "Unknown cpu used in --with-cpu=$with_cpu" 1>&2 >> exit 1 >> fi >> + >> + # see if --with-fpu matches any of the supported FPUs >> + case "$with_fpu" in >> + "") >> + # OK >> + ;; >> + fpus | fpus_div | fpus_fma | fpus_all) >> + # OK if em or hs >> + if ! grep -q -E "^ARC_CPU[[:blank:]]*\($new_cpu,[[:space:]]*[emhs]+," \ > you changed only the first :space: to :blank: (everywhere) > >> + ${srcdir}/config/arc/arc-cpus.def >> + then >> + echo "Unknown floating point type used in "\ >> + "--with-fpu=$with_fpu for cpu $new_cpu" 1>&2 >> + exit 1 >> + fi >> + ;; >> + fpuda | fpuda_div | fpuda_fma | fpuda_all) >> + # OK only em >> + if ! grep -q -E "^ARC_CPU[[:blank:]]*\($new_cpu,[[:space:]]*em," \ >> + ${srcdir}/config/arc/arc-cpus.def >> + then >> + echo "Unknown floating point type used in "\ >> + "--with-fpu=$with_fpu for cpu $new_cpu" 1>&2 >> + exit 1 >> + fi >> + ;; >> + fpud | fpud_div | fpud_fma | fpud_all) >> + # OK only hs >> + if ! grep -q -E "^ARC_CPU[[:blank:]]*\($new_cpu,[[:space:]]*hs," \ >> + ${srcdir}/config/arc/arc-cpus.def >> + then >> + echo "Unknown floating point type used in"\ >> + "--with-fpu=$with_fpu for cpu $new_cpu" 1>&2 > missing trailing space after 'in' in "used in"\ > > LGTM with that fixed with or without cutting down on grep lines, > but of course i cannot approve it. But your comments are greatly appreciated ;-) ACK'd for the trunk once the issues Bernhard mentioned are addressed. jeff
Hi Bernhard, Please find attached my latest patch, it includes (hopefully) all your feedback. Thank you for comments, Claudiu
On Fri, 11 Jun 2021 14:25:24 +0300 Claudiu Zissulescu <claziss@gmail.com> wrote: > Hi Bernhard, > > Please find attached my latest patch, it includes (hopefully) all your > feedback. > > Thank you for comments, concise and clean, i wouldn't know what to remove. LGTM. thanks for your patience!
On 6/13/2021 4:06 AM, Bernhard Reutner-Fischer wrote: > On Fri, 11 Jun 2021 14:25:24 +0300 > Claudiu Zissulescu <claziss@gmail.com> wrote: > >> Hi Bernhard, >> >> Please find attached my latest patch, it includes (hopefully) all your >> feedback. >> >> Thank you for comments, > concise and clean, i wouldn't know what to remove. LGTM. > thanks for your patience! THen let's consider it approved at this point. Thanks for chiming in Bernhard and thanks for implementing the suggestions Claudiu! jeff
Thanks a lot guys. Patch is pushed. //Claudiu On Mon, Jun 14, 2021 at 12:34 AM Jeff Law <jeffreyalaw@gmail.com> wrote: > > > > On 6/13/2021 4:06 AM, Bernhard Reutner-Fischer wrote: > > On Fri, 11 Jun 2021 14:25:24 +0300 > > Claudiu Zissulescu <claziss@gmail.com> wrote: > > > >> Hi Bernhard, > >> > >> Please find attached my latest patch, it includes (hopefully) all your > >> feedback. > >> > >> Thank you for comments, > > concise and clean, i wouldn't know what to remove. LGTM. > > thanks for your patience! > THen let's consider it approved at this point. Thanks for chiming in > Bernhard and thanks for implementing the suggestions Claudiu! > > jeff
diff --git a/gcc/config.gcc b/gcc/config.gcc index 610422fb29ee..f46b5e79af69 100644 --- a/gcc/config.gcc +++ b/gcc/config.gcc @@ -4258,18 +4258,70 @@ case "${target}" in ;; arc*-*-*) - supported_defaults="cpu" + supported_defaults="cpu fpu" + new_cpu=hs38_linux if [ x"$with_cpu" = x ] \ || grep "^ARC_CPU ($with_cpu," \ ${srcdir}/config/arc/arc-cpus.def \ > /dev/null; then # Ok - true + new_cpu=$with_cpu else echo "Unknown cpu used in --with-cpu=$with_cpu" 1>&2 exit 1 fi + + # see if --with-fpu matches any of the supported FPUs + case "$with_fpu" in + "") + # OK + ;; + fpus | fpus_div | fpus_fma | fpus_all) + # OK if em or hs + if grep -P "^ARC_CPU \($new_cpu,\s+[emhs]+," \ + ${srcdir}/config/arc/arc-cpus.def \ + > /dev/null; then + # OK + true + else + echo "Unknown floating point type used in "\ + "--with-fpu=$with_fpu for cpu $new_cpu" 1>&2 + exit 1 + fi + ;; + fpuda | fpuda_div | fpuda_fma | fpuda_all) + # OK only em + if grep -P "^ARC_CPU \($new_cpu,\s+em," \ + ${srcdir}/config/arc/arc-cpus.def \ + > /dev/null; then + # OK + true + else + echo "Unknown floating point type used in "\ + "--with-fpu=$with_fpu for cpu $new_cpu" 1>&2 + exit 1 + fi + ;; + fpud | fpud_div | fpud_fma | fpud_all) + # OK only hs + if grep -P "^ARC_CPU \($new_cpu,\s+hs," \ + ${srcdir}/config/arc/arc-cpus.def \ + > /dev/null; then + # OK + true + else + echo "Unknown floating point type used in"\ + "--with-fpu=$with_fpu for cpu $new_cpu" 1>&2 + exit 1 + fi + ;; + *) + echo "Unknown floating point type used in "\ + "--with-fpu=$with_fpu" 1>&2 + exit 1 + ;; + esac ;; csky-*-*) diff --git a/gcc/config/arc/arc.h b/gcc/config/arc/arc.h index 722bb10b8813..b9c4ba0398e5 100644 --- a/gcc/config/arc/arc.h +++ b/gcc/config/arc/arc.h @@ -100,7 +100,11 @@ extern const char *arc_cpu_to_as (int argc, const char **argv); "%:cpu_to_as(%{mcpu=*:%*}) %{mspfp*} %{mdpfp*} " \ "%{mfpu=fpuda*:-mfpuda} %{mcode-density}" +/* Support for a compile-time default CPU and FPU. The rules are: + --with-cpu is ignored if -mcpu, mARC*, marc*, mA7, mA6 are specified. + --with-fpu is ignored if -mfpu is specified. */ #define OPTION_DEFAULT_SPECS \ + {"fpu", "%{!mfpu=*:-mfpu=%(VALUE)}"}, \ {"cpu", "%{!mcpu=*:%{!mARC*:%{!marc*:%{!mA7:%{!mA6:-mcpu=%(VALUE)}}}}}" } #ifndef DRIVER_ENDIAN_SELF_SPECS
Hi Jeff, I would like to add spport for selecting the ARCv2 FPU extension at configuration-time. The --with-fpu configuration option is ignored when -mfpu compiler option is specified. My concern is using `grep -P` when configuring. Is that ok? Thanks, Claudiu gcc/ yyyy-mm-dd Claudiu Zissulescu <claziss@synopsys.com> * config.gcc (arc): Add support for with_cpu option. * config/arc/arc.h (OPTION_DEFAULT_SPECS): Add fpu. Signed-off-by: Claudiu Zissulescu <claziss@synopsys.com> --- gcc/config.gcc | 56 ++++++++++++++++++++++++++++++++++++++++++-- gcc/config/arc/arc.h | 4 ++++ 2 files changed, 58 insertions(+), 2 deletions(-)