Re: [PATCH v1 1/1] gcc: config: microblaze: fix cpu version check
Hi Mark,
> Le 23 oct. 2023 à 16:07, Hatle, Mark 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
>> ---
>> 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
Re: [PATCH v1 1/1] gcc: config: microblaze: fix cpu version check
> Le 23 oct. 2023 à 18:40, Michael Eager 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 >> --- >> 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
RE: [PATCH v1 1/1] gcc: config: microblaze: fix cpu version check
>>> 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
>>> ---
>>> 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 "" } 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
RE: [PATCH v1 1/1] gcc: config: microblaze: fix cpu version check
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
---
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 "" } 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
RE: [PATCH v6 1/1] gcc: config: microblaze: fix cpu version check
Hi Michael, > The MICROBLAZE_VERSION_COMPARE was incorrectly using strcasecmp > instead of strverscmp to check the mcpu version against feature > options. By simply changing the define to use strverscmp, the new > version 10.0 is treated correctly as a higher version than previous > versions. > > Signed-off-by: Neal Frager > Added to commit message; > Fix incorrect warning with -mcpu=10.0: > warning: '-mxl-multiply-high' can be used only with > '-mcpu=v6.00.a' or greater > --- > V1->V2: > - No need to create a new microblaze specific version check > routine as strverscmp is the correct solution. > V2->V3: > - Changed mcpu define for microblaze isa testsuite examples. > V3->V4: > - Added ChangeLog > V4->V5: > - Added testsuite ChangeLog > V5->V6: > - Updated testsuite ChangeLog to include all files > --- > gcc/ChangeLog | 4 > gcc/config/microblaze/microblaze.cc | 2 +- > gcc/testsuite/ChangeLog | 22 +++ > .../gcc.target/microblaze/isa/bshift.c| 2 +- > gcc/testsuite/gcc.target/microblaze/isa/div.c | 2 +- > .../gcc.target/microblaze/isa/fcmp1.c | 2 +- > .../gcc.target/microblaze/isa/fcmp2.c | 2 +- > .../gcc.target/microblaze/isa/fcmp3.c | 2 +- > .../gcc.target/microblaze/isa/fcmp4.c | 2 +- > .../gcc.target/microblaze/isa/fcvt.c | 2 +- > .../gcc.target/microblaze/isa/float.c | 2 +- > .../gcc.target/microblaze/isa/fsqrt.c | 2 +- > .../microblaze/isa/mul-bshift-pcmp.c | 2 +- > .../gcc.target/microblaze/isa/mul-bshift.c| 2 +- > gcc/testsuite/gcc.target/microblaze/isa/mul.c | 2 +- > .../microblaze/isa/mulh-bshift-pcmp.c | 2 +- > .../gcc.target/microblaze/isa/mulh.c | 2 +- > .../gcc.target/microblaze/isa/nofcmp.c| 2 +- > .../gcc.target/microblaze/isa/nofloat.c | 2 +- > .../gcc.target/microblaze/isa/pcmp.c | 2 +- > .../gcc.target/microblaze/isa/vanilla.c | 2 +- > .../gcc.target/microblaze/microblaze.exp | 2 +- > 22 files changed, 46 insertions(+), 20 deletions(-) > Committed. Did you commit this patch? I only see the ChangeLog files have been updated by your commit. Am I missing something? Best regards, Neal Frager AMD
RE: [PATCH v6 1/1] gcc: config: microblaze: fix cpu version check
> Hi Michael, > >> The MICROBLAZE_VERSION_COMPARE was incorrectly using strcasecmp >> instead of strverscmp to check the mcpu version against feature >> options. By simply changing the define to use strverscmp, the new >> version 10.0 is treated correctly as a higher version than previous >> versions. >> >> Signed-off-by: Neal Frager > >> Added to commit message; >> Fix incorrect warning with -mcpu=10.0: >>warning: '-mxl-multiply-high' can be used only with >>'-mcpu=v6.00.a' or greater > >> --- >> V1->V2: >>- No need to create a new microblaze specific version check >> routine as strverscmp is the correct solution. >> V2->V3: >>- Changed mcpu define for microblaze isa testsuite examples. >> V3->V4: >>- Added ChangeLog >> V4->V5: >>- Added testsuite ChangeLog >> V5->V6: >>- Updated testsuite ChangeLog to include all files >> --- >>gcc/ChangeLog | 4 >>gcc/config/microblaze/microblaze.cc | 2 +- >>gcc/testsuite/ChangeLog | 22 +++ >>.../gcc.target/microblaze/isa/bshift.c| 2 +- >>gcc/testsuite/gcc.target/microblaze/isa/div.c | 2 +- >>.../gcc.target/microblaze/isa/fcmp1.c | 2 +- >>.../gcc.target/microblaze/isa/fcmp2.c | 2 +- >>.../gcc.target/microblaze/isa/fcmp3.c | 2 +- >>.../gcc.target/microblaze/isa/fcmp4.c | 2 +- >>.../gcc.target/microblaze/isa/fcvt.c | 2 +- >>.../gcc.target/microblaze/isa/float.c | 2 +- >>.../gcc.target/microblaze/isa/fsqrt.c | 2 +- >>.../microblaze/isa/mul-bshift-pcmp.c | 2 +- >>.../gcc.target/microblaze/isa/mul-bshift.c| 2 +- >>gcc/testsuite/gcc.target/microblaze/isa/mul.c | 2 +- >>.../microblaze/isa/mulh-bshift-pcmp.c | 2 +- >>.../gcc.target/microblaze/isa/mulh.c | 2 +- >>.../gcc.target/microblaze/isa/nofcmp.c| 2 +- >>.../gcc.target/microblaze/isa/nofloat.c | 2 +- >>.../gcc.target/microblaze/isa/pcmp.c | 2 +- >>.../gcc.target/microblaze/isa/vanilla.c | 2 +- >>.../gcc.target/microblaze/microblaze.exp | 2 +- >>22 files changed, 46 insertions(+), 20 deletions(-) > >> Committed. > > Did you commit this patch? I only see the ChangeLog files have been > updated by your commit. > > Am I missing something? > > Updated. Thanks! Best regards, Neal Frager AMD
