| Message ID | 20231023054851.1205436-1-neal.frager@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1,1/1] gcc: config: microblaze: fix cpu version check | expand |
Not sure if this will work, but there is a strverscmp function in libiberty that I think will work. So the microblaze version compare could be done as: #define MICROBLAZE_VERSION_COMPARE(VA,VB) strvercmp(VA, VB) (I've not tried this, just remembered doing something similar in the past.) --Mark On 10/23/23 12:48 AM, Neal Frager wrote: > There is a microblaze cpu version 10.0 included in versal. If the > minor version is only a single digit, then the version comparison > will fail as version 10.0 will appear as 100 compared to version > 6.00 or 8.30 which will calculate to values 600 and 830. > > The issue can be seen when using the '-mcpu=10.0' option. > > With this fix, versions with a single digit minor number such as > 10.0 will be calculated as greater than versions with a smaller > major version number, but with two minor version digits. > > By applying this fix, several incorrect warning messages will no > longer be printed when building the versal plm application, such > as the warning message below: > > warning: '-mxl-multiply-high' can be used only with '-mcpu=v6.00.a' or greater > > Signed-off-by: Neal Frager <neal.frager@amd.com> > --- > gcc/config/microblaze/microblaze.cc | 164 +++++++++++++--------------- > 1 file changed, 76 insertions(+), 88 deletions(-) > > diff --git a/gcc/config/microblaze/microblaze.cc b/gcc/config/microblaze/microblaze.cc > index c9f6c4198cf..6e1555f6eb3 100644 > --- a/gcc/config/microblaze/microblaze.cc > +++ b/gcc/config/microblaze/microblaze.cc > @@ -56,8 +56,6 @@ > /* This file should be included last. */ > #include "target-def.h" > > -#define MICROBLAZE_VERSION_COMPARE(VA,VB) strcasecmp (VA, VB) > - > /* Classifies an address. > > ADDRESS_INVALID > @@ -1297,12 +1295,73 @@ microblaze_expand_block_move (rtx dest, rtx src, rtx length, rtx align_rtx) > return false; > } > > +/* Convert a version number of the form "vX.YY.Z" to an integer encoding > + for easier range comparison. */ > +static int > +microblaze_version_to_int (const char *version) > +{ > + const char *p, *v; > + const char *tmpl = "vXX.YY.Z"; > + int iver1 =0, iver2 =0, iver3 =0; > + > + p = version; > + v = tmpl; > + > + while (*p) > + { > + if (*v == 'X') > + { /* Looking for major */ > + if (*p == '.') > + *v++; > + else > + { > + if (!(*p >= '0' && *p <= '9')) > + return -1; > + iver1 += (int) (*p - '0'); > + iver1 *= 1000; > + } > + } > + else if (*v == 'Y') > + { /* Looking for minor */ > + if (!(*p >= '0' && *p <= '9')) > + return -1; > + iver2 += (int) (*p - '0'); > + iver2 *= 10; > + } > + else if (*v == 'Z') > + { /* Looking for compat */ > + if (!(*p >= 'a' && *p <= 'z')) > + return -1; > + iver3 = (int) (*p - 'a'); > + } > + else > + { > + if (*p != *v) > + return -1; > + } > + > + v++; > + p++; > + } > + > + if (*p) > + return -1; > + > + return iver1 + iver2 + iver3; > +} > + > static bool > microblaze_rtx_costs (rtx x, machine_mode mode, int outer_code ATTRIBUTE_UNUSED, > int opno ATTRIBUTE_UNUSED, int *total, > bool speed ATTRIBUTE_UNUSED) > { > int code = GET_CODE (x); > + int ver, ver_int; > + > + if (microblaze_select_cpu == NULL) > + microblaze_select_cpu = MICROBLAZE_DEFAULT_CPU; > + > + ver_int = microblaze_version_to_int (microblaze_select_cpu); > > switch (code) > { > @@ -1345,8 +1404,8 @@ microblaze_rtx_costs (rtx x, machine_mode mode, int outer_code ATTRIBUTE_UNUSED, > { > if (TARGET_BARREL_SHIFT) > { > - if (MICROBLAZE_VERSION_COMPARE (microblaze_select_cpu, "v5.00.a") > - >= 0) > + ver = ver_int - microblaze_version_to_int("v5.00.a"); > + if (ver >= 0) > *total = COSTS_N_INSNS (1); > else > *total = COSTS_N_INSNS (2); > @@ -1407,8 +1466,8 @@ microblaze_rtx_costs (rtx x, machine_mode mode, int outer_code ATTRIBUTE_UNUSED, > } > else if (!TARGET_SOFT_MUL) > { > - if (MICROBLAZE_VERSION_COMPARE (microblaze_select_cpu, "v5.00.a") > - >= 0) > + ver = ver_int - microblaze_version_to_int("v5.00.a"); > + if (ver >= 0) > *total = COSTS_N_INSNS (1); > else > *total = COSTS_N_INSNS (3); > @@ -1681,72 +1740,13 @@ function_arg_partial_bytes (cumulative_args_t cum_v, > return 0; > } > > -/* Convert a version number of the form "vX.YY.Z" to an integer encoding > - for easier range comparison. */ > -static int > -microblaze_version_to_int (const char *version) > -{ > - const char *p, *v; > - const char *tmpl = "vXX.YY.Z"; > - int iver = 0; > - > - p = version; > - v = tmpl; > - > - while (*p) > - { > - if (*v == 'X') > - { /* Looking for major */ > - if (*p == '.') > - { > - v++; > - } > - else > - { > - if (!(*p >= '0' && *p <= '9')) > - return -1; > - iver += (int) (*p - '0'); > - iver *= 10; > - } > - } > - else if (*v == 'Y') > - { /* Looking for minor */ > - if (!(*p >= '0' && *p <= '9')) > - return -1; > - iver += (int) (*p - '0'); > - iver *= 10; > - } > - else if (*v == 'Z') > - { /* Looking for compat */ > - if (!(*p >= 'a' && *p <= 'z')) > - return -1; > - iver *= 10; > - iver += (int) (*p - 'a'); > - } > - else > - { > - if (*p != *v) > - return -1; > - } > - > - v++; > - p++; > - } > - > - if (*p) > - return -1; > - > - return iver; > -} > - > - > static void > microblaze_option_override (void) > { > int i, start; > int regno; > machine_mode mode; > - int ver; > + int ver, ver_int; > > microblaze_section_threshold = (OPTION_SET_P (g_switch_value) > ? g_switch_value > @@ -1767,29 +1767,22 @@ microblaze_option_override (void) > /* Check the MicroBlaze CPU version for any special action to be done. */ > if (microblaze_select_cpu == NULL) > microblaze_select_cpu = MICROBLAZE_DEFAULT_CPU; > - ver = microblaze_version_to_int (microblaze_select_cpu); > - if (ver == -1) > + ver_int = microblaze_version_to_int (microblaze_select_cpu); > + if (ver_int == -1) > { > error ("%qs is an invalid argument to %<-mcpu=%>", microblaze_select_cpu); > } > > - ver = MICROBLAZE_VERSION_COMPARE (microblaze_select_cpu, "v3.00.a"); > + ver = ver_int - microblaze_version_to_int("v3.00.a"); > if (ver < 0) > { > /* No hardware exceptions in earlier versions. So no worries. */ > -#if 0 > - microblaze_select_flags &= ~(MICROBLAZE_MASK_NO_UNSAFE_DELAY); > -#endif > microblaze_no_unsafe_delay = 0; > microblaze_pipe = MICROBLAZE_PIPE_3; > } > else if (ver == 0 > - || (MICROBLAZE_VERSION_COMPARE (microblaze_select_cpu, "v4.00.b") > - == 0)) > + || (ver_int == microblaze_version_to_int("v4.00.b"))) > { > -#if 0 > - microblaze_select_flags |= (MICROBLAZE_MASK_NO_UNSAFE_DELAY); > -#endif > microblaze_no_unsafe_delay = 1; > microblaze_pipe = MICROBLAZE_PIPE_3; > } > @@ -1797,16 +1790,11 @@ microblaze_option_override (void) > { > /* We agree to use 5 pipe-stage model even on area optimized 3 > pipe-stage variants. */ > -#if 0 > - microblaze_select_flags &= ~(MICROBLAZE_MASK_NO_UNSAFE_DELAY); > -#endif > microblaze_no_unsafe_delay = 0; > microblaze_pipe = MICROBLAZE_PIPE_5; > - if (MICROBLAZE_VERSION_COMPARE (microblaze_select_cpu, "v5.00.a") == 0 > - || MICROBLAZE_VERSION_COMPARE (microblaze_select_cpu, > - "v5.00.b") == 0 > - || MICROBLAZE_VERSION_COMPARE (microblaze_select_cpu, > - "v5.00.c") == 0) > + if ((ver_int == microblaze_version_to_int("v5.00.a")) > + || (ver_int == microblaze_version_to_int("v5.00.b")) > + || (ver_int == microblaze_version_to_int("v5.00.c"))) > { > /* Pattern compares are to be turned on by default only when > compiling for MB v5.00.'z'. */ > @@ -1814,7 +1802,7 @@ microblaze_option_override (void) > } > } > > - ver = MICROBLAZE_VERSION_COMPARE (microblaze_select_cpu, "v6.00.a"); > + ver = ver_int - microblaze_version_to_int("v6.00.a"); > if (ver < 0) > { > if (TARGET_MULTIPLY_HIGH) > @@ -1823,7 +1811,7 @@ microblaze_option_override (void) > "%<-mcpu=v6.00.a%> or greater"); > } > > - ver = MICROBLAZE_VERSION_COMPARE (microblaze_select_cpu, "v8.10.a"); > + ver = ver_int - microblaze_version_to_int("v8.10.a"); > microblaze_has_clz = 1; > if (ver < 0) > { > @@ -1832,7 +1820,7 @@ microblaze_option_override (void) > } > > /* TARGET_REORDER defaults to 2 if -mxl-reorder not specified. */ > - ver = MICROBLAZE_VERSION_COMPARE (microblaze_select_cpu, "v8.30.a"); > + ver = ver_int - microblaze_version_to_int("v8.30.a"); > if (ver < 0) > { > if (TARGET_REORDER == 1)
Hi Mark, > Le 23 oct. 2023 à 16:07, Hatle, Mark <mark.hatle@amd.com> a écrit : > > Not sure if this will work, but there is a strverscmp function in libiberty that I think will work. > > So the microblaze version compare could be done as: > > #define MICROBLAZE_VERSION_COMPARE(VA,VB) strvercmp(VA, VB) > > (I've not tried this, just remembered doing something similar in the past.) > > --Mark Thank you for the good idea. I will have a look. The current version of the patch I submitted basically came from the meta-xilinx gcc patch 0024. If there is already a way to version compare, we probably never should have implemented our own routine to being with. I will check this out, and submit a v2 for this patch, if it works. Best regards, Neal Frager AMD > >> On 10/23/23 12:48 AM, Neal Frager wrote: >> There is a microblaze cpu version 10.0 included in versal. If the >> minor version is only a single digit, then the version comparison >> will fail as version 10.0 will appear as 100 compared to version >> 6.00 or 8.30 which will calculate to values 600 and 830. >> The issue can be seen when using the '-mcpu=10.0' option. >> With this fix, versions with a single digit minor number such as >> 10.0 will be calculated as greater than versions with a smaller >> major version number, but with two minor version digits. >> By applying this fix, several incorrect warning messages will no >> longer be printed when building the versal plm application, such >> as the warning message below: >> warning: '-mxl-multiply-high' can be used only with '-mcpu=v6.00.a' or greater >> Signed-off-by: Neal Frager <neal.frager@amd.com> >> --- >> gcc/config/microblaze/microblaze.cc | 164 +++++++++++++--------------- >> 1 file changed, 76 insertions(+), 88 deletions(-) >> diff --git a/gcc/config/microblaze/microblaze.cc b/gcc/config/microblaze/microblaze.cc >> index c9f6c4198cf..6e1555f6eb3 100644 >> --- a/gcc/config/microblaze/microblaze.cc >> +++ b/gcc/config/microblaze/microblaze.cc >> @@ -56,8 +56,6 @@ >> /* This file should be included last. */ >> #include "target-def.h" >> -#define MICROBLAZE_VERSION_COMPARE(VA,VB) strcasecmp (VA, VB) >> - >> /* Classifies an address. >> ADDRESS_INVALID >> @@ -1297,12 +1295,73 @@ microblaze_expand_block_move (rtx dest, rtx src, rtx length, rtx align_rtx) >> return false; >> } >> +/* Convert a version number of the form "vX.YY.Z" to an integer encoding >> + for easier range comparison. */ >> +static int >> +microblaze_version_to_int (const char *version) >> +{ >> + const char *p, *v; >> + const char *tmpl = "vXX.YY.Z"; >> + int iver1 =0, iver2 =0, iver3 =0; >> + >> + p = version; >> + v = tmpl; >> + >> + while (*p) >> + { >> + if (*v == 'X') >> + { /* Looking for major */ >> + if (*p == '.') >> + *v++; >> + else >> + { >> + if (!(*p >= '0' && *p <= '9')) >> + return -1; >> + iver1 += (int) (*p - '0'); >> + iver1 *= 1000; >> + } >> + } >> + else if (*v == 'Y') >> + { /* Looking for minor */ >> + if (!(*p >= '0' && *p <= '9')) >> + return -1; >> + iver2 += (int) (*p - '0'); >> + iver2 *= 10; >> + } >> + else if (*v == 'Z') >> + { /* Looking for compat */ >> + if (!(*p >= 'a' && *p <= 'z')) >> + return -1; >> + iver3 = (int) (*p - 'a'); >> + } >> + else >> + { >> + if (*p != *v) >> + return -1; >> + } >> + >> + v++; >> + p++; >> + } >> + >> + if (*p) >> + return -1; >> + >> + return iver1 + iver2 + iver3; >> +} >> + >> static bool >> microblaze_rtx_costs (rtx x, machine_mode mode, int outer_code ATTRIBUTE_UNUSED, >> int opno ATTRIBUTE_UNUSED, int *total, >> bool speed ATTRIBUTE_UNUSED) >> { >> int code = GET_CODE (x); >> + int ver, ver_int; >> + >> + if (microblaze_select_cpu == NULL) >> + microblaze_select_cpu = MICROBLAZE_DEFAULT_CPU; >> + >> + ver_int = microblaze_version_to_int (microblaze_select_cpu); >> switch (code) >> { >> @@ -1345,8 +1404,8 @@ microblaze_rtx_costs (rtx x, machine_mode mode, int outer_code ATTRIBUTE_UNUSED, >> { >> if (TARGET_BARREL_SHIFT) >> { >> - if (MICROBLAZE_VERSION_COMPARE (microblaze_select_cpu, "v5.00.a") >> - >= 0) >> + ver = ver_int - microblaze_version_to_int("v5.00.a"); >> + if (ver >= 0) >> *total = COSTS_N_INSNS (1); >> else >> *total = COSTS_N_INSNS (2); >> @@ -1407,8 +1466,8 @@ microblaze_rtx_costs (rtx x, machine_mode mode, int outer_code ATTRIBUTE_UNUSED, >> } >> else if (!TARGET_SOFT_MUL) >> { >> - if (MICROBLAZE_VERSION_COMPARE (microblaze_select_cpu, "v5.00.a") >> - >= 0) >> + ver = ver_int - microblaze_version_to_int("v5.00.a"); >> + if (ver >= 0) >> *total = COSTS_N_INSNS (1); >> else >> *total = COSTS_N_INSNS (3); >> @@ -1681,72 +1740,13 @@ function_arg_partial_bytes (cumulative_args_t cum_v, >> return 0; >> } >> -/* Convert a version number of the form "vX.YY.Z" to an integer encoding >> - for easier range comparison. */ >> -static int >> -microblaze_version_to_int (const char *version) >> -{ >> - const char *p, *v; >> - const char *tmpl = "vXX.YY.Z"; >> - int iver = 0; >> - >> - p = version; >> - v = tmpl; >> - >> - while (*p) >> - { >> - if (*v == 'X') >> - { /* Looking for major */ >> - if (*p == '.') >> - { >> - v++; >> - } >> - else >> - { >> - if (!(*p >= '0' && *p <= '9')) >> - return -1; >> - iver += (int) (*p - '0'); >> - iver *= 10; >> - } >> - } >> - else if (*v == 'Y') >> - { /* Looking for minor */ >> - if (!(*p >= '0' && *p <= '9')) >> - return -1; >> - iver += (int) (*p - '0'); >> - iver *= 10; >> - } >> - else if (*v == 'Z') >> - { /* Looking for compat */ >> - if (!(*p >= 'a' && *p <= 'z')) >> - return -1; >> - iver *= 10; >> - iver += (int) (*p - 'a'); >> - } >> - else >> - { >> - if (*p != *v) >> - return -1; >> - } >> - >> - v++; >> - p++; >> - } >> - >> - if (*p) >> - return -1; >> - >> - return iver; >> -} >> - >> - >> static void >> microblaze_option_override (void) >> { >> int i, start; >> int regno; >> machine_mode mode; >> - int ver; >> + int ver, ver_int; >> microblaze_section_threshold = (OPTION_SET_P (g_switch_value) >> ? g_switch_value >> @@ -1767,29 +1767,22 @@ microblaze_option_override (void) >> /* Check the MicroBlaze CPU version for any special action to be done. */ >> if (microblaze_select_cpu == NULL) >> microblaze_select_cpu = MICROBLAZE_DEFAULT_CPU; >> - ver = microblaze_version_to_int (microblaze_select_cpu); >> - if (ver == -1) >> + ver_int = microblaze_version_to_int (microblaze_select_cpu); >> + if (ver_int == -1) >> { >> error ("%qs is an invalid argument to %<-mcpu=%>", microblaze_select_cpu); >> } >> - ver = MICROBLAZE_VERSION_COMPARE (microblaze_select_cpu, "v3.00.a"); >> + ver = ver_int - microblaze_version_to_int("v3.00.a"); >> if (ver < 0) >> { >> /* No hardware exceptions in earlier versions. So no worries. */ >> -#if 0 >> - microblaze_select_flags &= ~(MICROBLAZE_MASK_NO_UNSAFE_DELAY); >> -#endif >> microblaze_no_unsafe_delay = 0; >> microblaze_pipe = MICROBLAZE_PIPE_3; >> } >> else if (ver == 0 >> - || (MICROBLAZE_VERSION_COMPARE (microblaze_select_cpu, "v4.00.b") >> - == 0)) >> + || (ver_int == microblaze_version_to_int("v4.00.b"))) >> { >> -#if 0 >> - microblaze_select_flags |= (MICROBLAZE_MASK_NO_UNSAFE_DELAY); >> -#endif >> microblaze_no_unsafe_delay = 1; >> microblaze_pipe = MICROBLAZE_PIPE_3; >> } >> @@ -1797,16 +1790,11 @@ microblaze_option_override (void) >> { >> /* We agree to use 5 pipe-stage model even on area optimized 3 >> pipe-stage variants. */ >> -#if 0 >> - microblaze_select_flags &= ~(MICROBLAZE_MASK_NO_UNSAFE_DELAY); >> -#endif >> microblaze_no_unsafe_delay = 0; >> microblaze_pipe = MICROBLAZE_PIPE_5; >> - if (MICROBLAZE_VERSION_COMPARE (microblaze_select_cpu, "v5.00.a") == 0 >> - || MICROBLAZE_VERSION_COMPARE (microblaze_select_cpu, >> - "v5.00.b") == 0 >> - || MICROBLAZE_VERSION_COMPARE (microblaze_select_cpu, >> - "v5.00.c") == 0) >> + if ((ver_int == microblaze_version_to_int("v5.00.a")) >> + || (ver_int == microblaze_version_to_int("v5.00.b")) >> + || (ver_int == microblaze_version_to_int("v5.00.c"))) >> { >> /* Pattern compares are to be turned on by default only when >> compiling for MB v5.00.'z'. */ >> @@ -1814,7 +1802,7 @@ microblaze_option_override (void) >> } >> } >> - ver = MICROBLAZE_VERSION_COMPARE (microblaze_select_cpu, "v6.00.a"); >> + ver = ver_int - microblaze_version_to_int("v6.00.a"); >> if (ver < 0) >> { >> if (TARGET_MULTIPLY_HIGH) >> @@ -1823,7 +1811,7 @@ microblaze_option_override (void) >> "%<-mcpu=v6.00.a%> or greater"); >> } >> - ver = MICROBLAZE_VERSION_COMPARE (microblaze_select_cpu, "v8.10.a"); >> + ver = ver_int - microblaze_version_to_int("v8.10.a"); >> microblaze_has_clz = 1; >> if (ver < 0) >> { >> @@ -1832,7 +1820,7 @@ microblaze_option_override (void) >> } >> /* TARGET_REORDER defaults to 2 if -mxl-reorder not specified. */ >> - ver = MICROBLAZE_VERSION_COMPARE (microblaze_select_cpu, "v8.30.a"); >> + ver = ver_int - microblaze_version_to_int("v8.30.a"); >> if (ver < 0) >> { >> if (TARGET_REORDER == 1)
On 10/22/23 22:48, Neal Frager wrote: > There is a microblaze cpu version 10.0 included in versal. If the > minor version is only a single digit, then the version comparison > will fail as version 10.0 will appear as 100 compared to version > 6.00 or 8.30 which will calculate to values 600 and 830. > > The issue can be seen when using the '-mcpu=10.0' option. > > With this fix, versions with a single digit minor number such as > 10.0 will be calculated as greater than versions with a smaller > major version number, but with two minor version digits. > > By applying this fix, several incorrect warning messages will no > longer be printed when building the versal plm application, such > as the warning message below: > > warning: '-mxl-multiply-high' can be used only with '-mcpu=v6.00.a' or greater > > Signed-off-by: Neal Frager <neal.frager@amd.com> > --- > gcc/config/microblaze/microblaze.cc | 164 +++++++++++++--------------- > 1 file changed, 76 insertions(+), 88 deletions(-) Please add a test case.
> Le 23 oct. 2023 à 18:40, Michael Eager <eager@eagercon.com> a écrit : > > On 10/22/23 22:48, Neal Frager wrote: >> There is a microblaze cpu version 10.0 included in versal. If the >> minor version is only a single digit, then the version comparison >> will fail as version 10.0 will appear as 100 compared to version >> 6.00 or 8.30 which will calculate to values 600 and 830. >> The issue can be seen when using the '-mcpu=10.0' option. >> With this fix, versions with a single digit minor number such as >> 10.0 will be calculated as greater than versions with a smaller >> major version number, but with two minor version digits. >> By applying this fix, several incorrect warning messages will no >> longer be printed when building the versal plm application, such >> as the warning message below: >> warning: '-mxl-multiply-high' can be used only with '-mcpu=v6.00.a' or greater >> Signed-off-by: Neal Frager <neal.frager@amd.com> >> --- >> gcc/config/microblaze/microblaze.cc | 164 +++++++++++++--------------- >> 1 file changed, 76 insertions(+), 88 deletions(-) > > Please add a test case. > > -- > Michael Eager Hi Michael, Would you mind helping me understand how to make a gcc test case for this patch? This patch does not change the resulting binaries of a microblaze gcc build. The output will be the same with our without the patch, so I do not having anything in the binary itself to verify. All that happens is false warning messages will not be printed when building with ‘-mcpu=10.0’. Is there a way to test for warning messages? In any case, please do not commit v1 of this patch. I am going to work on making a v2 based on Mark’s feedback. Thanks for your help! Best regards, Neal Frager AMD
On 10/23/23 11:37, Frager, Neal wrote: > > > >> Le 23 oct. 2023 à 18:40, Michael Eager <eager@eagercon.com> a écrit : >> >> On 10/22/23 22:48, Neal Frager wrote: >>> There is a microblaze cpu version 10.0 included in versal. If the >>> minor version is only a single digit, then the version comparison >>> will fail as version 10.0 will appear as 100 compared to version >>> 6.00 or 8.30 which will calculate to values 600 and 830. >>> The issue can be seen when using the '-mcpu=10.0' option. >>> With this fix, versions with a single digit minor number such as >>> 10.0 will be calculated as greater than versions with a smaller >>> major version number, but with two minor version digits. >>> By applying this fix, several incorrect warning messages will no >>> longer be printed when building the versal plm application, such >>> as the warning message below: >>> warning: '-mxl-multiply-high' can be used only with '-mcpu=v6.00.a' or greater >>> Signed-off-by: Neal Frager <neal.frager@amd.com> >>> --- >>> gcc/config/microblaze/microblaze.cc | 164 +++++++++++++--------------- >>> 1 file changed, 76 insertions(+), 88 deletions(-) >> >> Please add a test case. >> >> -- >> Michael Eager > > Hi Michael, > > Would you mind helping me understand how to make a gcc test case for this patch? > > This patch does not change the resulting binaries of a microblaze gcc build. The output will be the same with our without the patch, so I do not having anything in the binary itself to verify. > > All that happens is false warning messages will not be printed when building with ‘-mcpu=10.0’. Is there a way to test for warning messages? > > In any case, please do not commit v1 of this patch. I am going to work on making a v2 based on Mark’s feedback. You can create a test case which passes the -mcpu=10.0 and other options to GCC and verify that the message is not generated after the patch is applied. You can make all GCC warnings into errors with the "-Werror" option. This means that the compile will fail if the warning is issued. Take a look at gcc/testsuite/gcc.target/aarch64/bti-1.c for an example of using { dg-options "<opt>" } to specify command line options. There is a test suite option (dg-warning) which checks that a particular source line generates a warning message, but it isn't clear whether is is possible to check that a warning is not issued.
>>> There is a microblaze cpu version 10.0 included in versal. If the >>> minor version is only a single digit, then the version comparison >>> will fail as version 10.0 will appear as 100 compared to version >>> 6.00 or 8.30 which will calculate to values 600 and 830. >>> The issue can be seen when using the '-mcpu=10.0' option. >>> With this fix, versions with a single digit minor number such as >>> 10.0 will be calculated as greater than versions with a smaller >>> major version number, but with two minor version digits. >>> By applying this fix, several incorrect warning messages will no >>> longer be printed when building the versal plm application, such as >>> the warning message below: >>> warning: '-mxl-multiply-high' can be used only with '-mcpu=v6.00.a' >>> or greater >>> Signed-off-by: Neal Frager <neal.frager@amd.com> >>> --- >>> gcc/config/microblaze/microblaze.cc | 164 +++++++++++++--------------- >>> 1 file changed, 76 insertions(+), 88 deletions(-) >> >> Please add a test case. >> >> -- >> Michael Eager > > Hi Michael, > > Would you mind helping me understand how to make a gcc test case for this patch? > > This patch does not change the resulting binaries of a microblaze gcc build. The output will be the same with our without the patch, so I do not having anything in the binary itself to verify. > > All that happens is false warning messages will not be printed when building with ‘-mcpu=10.0’. Is there a way to test for warning messages? > > In any case, please do not commit v1 of this patch. I am going to work on making a v2 based on Mark’s feedback. > You can create a test case which passes the -mcpu=10.0 and other options to GCC and verify that the message is not generated after the patch is applied. > You can make all GCC warnings into errors with the "-Werror" option. > This means that the compile will fail if the warning is issued. > Take a look at gcc/testsuite/gcc.target/aarch64/bti-1.c for an example of using { dg-options "<opt>" } to specify command line options. > There is a test suite option (dg-warning) which checks that a particular source line generates a warning message, but it isn't clear whether is is possible to check that a warning is not issued. Hi Michael, Thanks to Mark Hatle's feedback, we have a much simpler solution to the problem. The following change is actually all that is necessary. Since we are just moving from strcasecmp to strverscmp, does v2 of the patch need to have a test case to go with it? -#define MICROBLAZE_VERSION_COMPARE(VA,VB) strcasecmp (VA, VB) +#define MICROBLAZE_VERSION_COMPARE(VA,VB) strverscmp (VA, VB) I assume there are already test cases that verify that strverscmp works correctly? Best regards, Neal Frager AMD
On 10/24/23 00:01, Frager, Neal wrote: >>>> There is a microblaze cpu version 10.0 included in versal. If the >>>> minor version is only a single digit, then the version comparison >>>> will fail as version 10.0 will appear as 100 compared to version >>>> 6.00 or 8.30 which will calculate to values 600 and 830. >>>> The issue can be seen when using the '-mcpu=10.0' option. >>>> With this fix, versions with a single digit minor number such as >>>> 10.0 will be calculated as greater than versions with a smaller >>>> major version number, but with two minor version digits. >>>> By applying this fix, several incorrect warning messages will no >>>> longer be printed when building the versal plm application, such as >>>> the warning message below: >>>> warning: '-mxl-multiply-high' can be used only with '-mcpu=v6.00.a' >>>> or greater >>>> Signed-off-by: Neal Frager <neal.frager@amd.com> >>>> --- >>>> gcc/config/microblaze/microblaze.cc | 164 +++++++++++++--------------- >>>> 1 file changed, 76 insertions(+), 88 deletions(-) >>> >>> Please add a test case. >>> >>> -- >>> Michael Eager >> >> Hi Michael, >> >> Would you mind helping me understand how to make a gcc test case for this patch? >> >> This patch does not change the resulting binaries of a microblaze gcc build. The output will be the same with our without the patch, so I do not having anything in the binary itself to verify. >> >> All that happens is false warning messages will not be printed when building with ‘-mcpu=10.0’. Is there a way to test for warning messages? >> >> In any case, please do not commit v1 of this patch. I am going to work on making a v2 based on Mark’s feedback. > >> You can create a test case which passes the -mcpu=10.0 and other options to GCC and verify that the message is not generated after the patch is applied. > >> You can make all GCC warnings into errors with the "-Werror" option. >> This means that the compile will fail if the warning is issued. > >> Take a look at gcc/testsuite/gcc.target/aarch64/bti-1.c for an example of using { dg-options "<opt>" } to specify command line options. > >> There is a test suite option (dg-warning) which checks that a particular source line generates a warning message, but it isn't clear whether is is possible to check that a warning is not issued. > > Hi Michael, > > Thanks to Mark Hatle's feedback, we have a much simpler solution to the problem. > > The following change is actually all that is necessary. Since we are just moving from > strcasecmp to strverscmp, does v2 of the patch need to have a test case to go with it? > > -#define MICROBLAZE_VERSION_COMPARE(VA,VB) strcasecmp (VA, VB) > +#define MICROBLAZE_VERSION_COMPARE(VA,VB) strverscmp (VA, VB) > > I assume there are already test cases that verify that strverscmp works correctly? Still need a test case to verify this fix.
>>>> There is a microblaze cpu version 10.0 included in versal. If the >>>> minor version is only a single digit, then the version comparison >>>> will fail as version 10.0 will appear as 100 compared to version >>>> 6.00 or 8.30 which will calculate to values 600 and 830. >>>> The issue can be seen when using the '-mcpu=10.0' option. >>>> With this fix, versions with a single digit minor number such as >>>> 10.0 will be calculated as greater than versions with a smaller >>>> major version number, but with two minor version digits. >>>> By applying this fix, several incorrect warning messages will no >>>> longer be printed when building the versal plm application, such as >>>> the warning message below: >>>> warning: '-mxl-multiply-high' can be used only with '-mcpu=v6.00.a' >>>> or greater >>>> Signed-off-by: Neal Frager <neal.frager@amd.com> >>>> --- >>>> gcc/config/microblaze/microblaze.cc | 164 +++++++++++++--------------- >>>> 1 file changed, 76 insertions(+), 88 deletions(-) >>> >>> Please add a test case. >>> >>> -- >>> Michael Eager >> >> Hi Michael, >> >> Would you mind helping me understand how to make a gcc test case for this patch? >> >> This patch does not change the resulting binaries of a microblaze gcc build. The output will be the same with our without the patch, so I do not having anything in the binary itself to verify. >> >> All that happens is false warning messages will not be printed when building with ‘-mcpu=10.0’. Is there a way to test for warning messages? >> >> In any case, please do not commit v1 of this patch. I am going to work on making a v2 based on Mark’s feedback. > >> You can create a test case which passes the -mcpu=10.0 and other options to GCC and verify that the message is not generated after the patch is applied. > >> You can make all GCC warnings into errors with the "-Werror" option. >> This means that the compile will fail if the warning is issued. > >> Take a look at gcc/testsuite/gcc.target/aarch64/bti-1.c for an example of using { dg-options "<opt>" } to specify command line options. > >> There is a test suite option (dg-warning) which checks that a particular source line generates a warning message, but it isn't clear whether is is possible to check that a warning is not issued. > > Hi Michael, > > Thanks to Mark Hatle's feedback, we have a much simpler solution to the problem. > > The following change is actually all that is necessary. Since we are > just moving from strcasecmp to strverscmp, does v2 of the patch need to have a test case to go with it? > > -#define MICROBLAZE_VERSION_COMPARE(VA,VB) strcasecmp (VA, VB) > +#define MICROBLAZE_VERSION_COMPARE(VA,VB) strverscmp (VA, VB) > > I assume there are already test cases that verify that strverscmp works correctly? > Still need a test case to verify this fix. Hi Michael, It appears to me that simply increasing the microblaze testsuite option from -mcpu=6.00.a to -mcpu=10.0 works for verifying this fix. Without the fix, the microblaze testsuite isa examples print the false warning messages when -mcpu=10.0 is used. Please see v3 of my patch which includes the testsuite update. Best regards, Neal Frager AMD
diff --git a/gcc/config/microblaze/microblaze.cc b/gcc/config/microblaze/microblaze.cc index c9f6c4198cf..6e1555f6eb3 100644 --- a/gcc/config/microblaze/microblaze.cc +++ b/gcc/config/microblaze/microblaze.cc @@ -56,8 +56,6 @@ /* This file should be included last. */ #include "target-def.h" -#define MICROBLAZE_VERSION_COMPARE(VA,VB) strcasecmp (VA, VB) - /* Classifies an address. ADDRESS_INVALID @@ -1297,12 +1295,73 @@ microblaze_expand_block_move (rtx dest, rtx src, rtx length, rtx align_rtx) return false; } +/* Convert a version number of the form "vX.YY.Z" to an integer encoding + for easier range comparison. */ +static int +microblaze_version_to_int (const char *version) +{ + const char *p, *v; + const char *tmpl = "vXX.YY.Z"; + int iver1 =0, iver2 =0, iver3 =0; + + p = version; + v = tmpl; + + while (*p) + { + if (*v == 'X') + { /* Looking for major */ + if (*p == '.') + *v++; + else + { + if (!(*p >= '0' && *p <= '9')) + return -1; + iver1 += (int) (*p - '0'); + iver1 *= 1000; + } + } + else if (*v == 'Y') + { /* Looking for minor */ + if (!(*p >= '0' && *p <= '9')) + return -1; + iver2 += (int) (*p - '0'); + iver2 *= 10; + } + else if (*v == 'Z') + { /* Looking for compat */ + if (!(*p >= 'a' && *p <= 'z')) + return -1; + iver3 = (int) (*p - 'a'); + } + else + { + if (*p != *v) + return -1; + } + + v++; + p++; + } + + if (*p) + return -1; + + return iver1 + iver2 + iver3; +} + static bool microblaze_rtx_costs (rtx x, machine_mode mode, int outer_code ATTRIBUTE_UNUSED, int opno ATTRIBUTE_UNUSED, int *total, bool speed ATTRIBUTE_UNUSED) { int code = GET_CODE (x); + int ver, ver_int; + + if (microblaze_select_cpu == NULL) + microblaze_select_cpu = MICROBLAZE_DEFAULT_CPU; + + ver_int = microblaze_version_to_int (microblaze_select_cpu); switch (code) { @@ -1345,8 +1404,8 @@ microblaze_rtx_costs (rtx x, machine_mode mode, int outer_code ATTRIBUTE_UNUSED, { if (TARGET_BARREL_SHIFT) { - if (MICROBLAZE_VERSION_COMPARE (microblaze_select_cpu, "v5.00.a") - >= 0) + ver = ver_int - microblaze_version_to_int("v5.00.a"); + if (ver >= 0) *total = COSTS_N_INSNS (1); else *total = COSTS_N_INSNS (2); @@ -1407,8 +1466,8 @@ microblaze_rtx_costs (rtx x, machine_mode mode, int outer_code ATTRIBUTE_UNUSED, } else if (!TARGET_SOFT_MUL) { - if (MICROBLAZE_VERSION_COMPARE (microblaze_select_cpu, "v5.00.a") - >= 0) + ver = ver_int - microblaze_version_to_int("v5.00.a"); + if (ver >= 0) *total = COSTS_N_INSNS (1); else *total = COSTS_N_INSNS (3); @@ -1681,72 +1740,13 @@ function_arg_partial_bytes (cumulative_args_t cum_v, return 0; } -/* Convert a version number of the form "vX.YY.Z" to an integer encoding - for easier range comparison. */ -static int -microblaze_version_to_int (const char *version) -{ - const char *p, *v; - const char *tmpl = "vXX.YY.Z"; - int iver = 0; - - p = version; - v = tmpl; - - while (*p) - { - if (*v == 'X') - { /* Looking for major */ - if (*p == '.') - { - v++; - } - else - { - if (!(*p >= '0' && *p <= '9')) - return -1; - iver += (int) (*p - '0'); - iver *= 10; - } - } - else if (*v == 'Y') - { /* Looking for minor */ - if (!(*p >= '0' && *p <= '9')) - return -1; - iver += (int) (*p - '0'); - iver *= 10; - } - else if (*v == 'Z') - { /* Looking for compat */ - if (!(*p >= 'a' && *p <= 'z')) - return -1; - iver *= 10; - iver += (int) (*p - 'a'); - } - else - { - if (*p != *v) - return -1; - } - - v++; - p++; - } - - if (*p) - return -1; - - return iver; -} - - static void microblaze_option_override (void) { int i, start; int regno; machine_mode mode; - int ver; + int ver, ver_int; microblaze_section_threshold = (OPTION_SET_P (g_switch_value) ? g_switch_value @@ -1767,29 +1767,22 @@ microblaze_option_override (void) /* Check the MicroBlaze CPU version for any special action to be done. */ if (microblaze_select_cpu == NULL) microblaze_select_cpu = MICROBLAZE_DEFAULT_CPU; - ver = microblaze_version_to_int (microblaze_select_cpu); - if (ver == -1) + ver_int = microblaze_version_to_int (microblaze_select_cpu); + if (ver_int == -1) { error ("%qs is an invalid argument to %<-mcpu=%>", microblaze_select_cpu); } - ver = MICROBLAZE_VERSION_COMPARE (microblaze_select_cpu, "v3.00.a"); + ver = ver_int - microblaze_version_to_int("v3.00.a"); if (ver < 0) { /* No hardware exceptions in earlier versions. So no worries. */ -#if 0 - microblaze_select_flags &= ~(MICROBLAZE_MASK_NO_UNSAFE_DELAY); -#endif microblaze_no_unsafe_delay = 0; microblaze_pipe = MICROBLAZE_PIPE_3; } else if (ver == 0 - || (MICROBLAZE_VERSION_COMPARE (microblaze_select_cpu, "v4.00.b") - == 0)) + || (ver_int == microblaze_version_to_int("v4.00.b"))) { -#if 0 - microblaze_select_flags |= (MICROBLAZE_MASK_NO_UNSAFE_DELAY); -#endif microblaze_no_unsafe_delay = 1; microblaze_pipe = MICROBLAZE_PIPE_3; } @@ -1797,16 +1790,11 @@ microblaze_option_override (void) { /* We agree to use 5 pipe-stage model even on area optimized 3 pipe-stage variants. */ -#if 0 - microblaze_select_flags &= ~(MICROBLAZE_MASK_NO_UNSAFE_DELAY); -#endif microblaze_no_unsafe_delay = 0; microblaze_pipe = MICROBLAZE_PIPE_5; - if (MICROBLAZE_VERSION_COMPARE (microblaze_select_cpu, "v5.00.a") == 0 - || MICROBLAZE_VERSION_COMPARE (microblaze_select_cpu, - "v5.00.b") == 0 - || MICROBLAZE_VERSION_COMPARE (microblaze_select_cpu, - "v5.00.c") == 0) + if ((ver_int == microblaze_version_to_int("v5.00.a")) + || (ver_int == microblaze_version_to_int("v5.00.b")) + || (ver_int == microblaze_version_to_int("v5.00.c"))) { /* Pattern compares are to be turned on by default only when compiling for MB v5.00.'z'. */ @@ -1814,7 +1802,7 @@ microblaze_option_override (void) } } - ver = MICROBLAZE_VERSION_COMPARE (microblaze_select_cpu, "v6.00.a"); + ver = ver_int - microblaze_version_to_int("v6.00.a"); if (ver < 0) { if (TARGET_MULTIPLY_HIGH) @@ -1823,7 +1811,7 @@ microblaze_option_override (void) "%<-mcpu=v6.00.a%> or greater"); } - ver = MICROBLAZE_VERSION_COMPARE (microblaze_select_cpu, "v8.10.a"); + ver = ver_int - microblaze_version_to_int("v8.10.a"); microblaze_has_clz = 1; if (ver < 0) { @@ -1832,7 +1820,7 @@ microblaze_option_override (void) } /* TARGET_REORDER defaults to 2 if -mxl-reorder not specified. */ - ver = MICROBLAZE_VERSION_COMPARE (microblaze_select_cpu, "v8.30.a"); + ver = ver_int - microblaze_version_to_int("v8.30.a"); if (ver < 0) { if (TARGET_REORDER == 1)
There is a microblaze cpu version 10.0 included in versal. If the minor version is only a single digit, then the version comparison will fail as version 10.0 will appear as 100 compared to version 6.00 or 8.30 which will calculate to values 600 and 830. The issue can be seen when using the '-mcpu=10.0' option. With this fix, versions with a single digit minor number such as 10.0 will be calculated as greater than versions with a smaller major version number, but with two minor version digits. By applying this fix, several incorrect warning messages will no longer be printed when building the versal plm application, such as the warning message below: warning: '-mxl-multiply-high' can be used only with '-mcpu=v6.00.a' or greater Signed-off-by: Neal Frager <neal.frager@amd.com> --- gcc/config/microblaze/microblaze.cc | 164 +++++++++++++--------------- 1 file changed, 76 insertions(+), 88 deletions(-)