diff mbox series

arm: Make arm_noce_conversion_profitable_p call default hook [PR 116444]

Message ID 2f10481d-0beb-4cc8-9230-610de2a76937@arm.com
State New
Headers show
Series arm: Make arm_noce_conversion_profitable_p call default hook [PR 116444] | expand

Commit Message

Andre Vieira (lists) Oct. 4, 2024, 3:55 p.m. UTC 
Hi,

The patch for 'arm: Fix missed CE optimization for armv8.1-m.main [PR 
116444]' introduced regressions with arm targets that used 'noce' before.
This is because it would approve all noce optimisations without using 
the default cost check. Not sure why this didn't show up in my original 
testing, I suspect you need to test this for a set of specific targets 
like Torbjorn did, thank you for pointing these issues out to me.

Could I ask you to rerun them with this patch? I'll try to do that 
locally too.

Happy to receive reviews, but I'm waiting for Torbjorn and my own 
testing to complete before committing.

When not dealing with the special armv8.1-m.main conditional 
instructions case
make sure it uses the default_noce_conversion_profitable_p call to determine
whether the sequence is cost effective.

gcc/ChangeLog:


	PR target/116444
	* config/arm/arm.cc (arm_noce_conversion_profitable_p): Call
	default_noce_conversion_profitable_p when not dealing with the
	armv8.1-m.main conditional instructions special cases.

Comments

Ramana Radhakrishnan Oct. 7, 2024, 4:39 a.m. UTC | #1 
On Fri, Oct 4, 2024 at 9:25 PM Andre Vieira (lists)
<andre.simoesdiasvieira@arm.com> wrote:
>
> Hi,
>
> The patch for 'arm: Fix missed CE optimization for armv8.1-m.main [PR
> 116444]' introduced regressions with arm targets that used 'noce' before.
> This is because it would approve all noce optimisations without using
> the default cost check. Not sure why this didn't show up in my original
> testing, I suspect you need to test this for a set of specific targets
> like Torbjorn did, thank you for pointing these issues out to me.
>
> Could I ask you to rerun them with this patch? I'll try to do that
> locally too.
>
> Happy to receive reviews, but I'm waiting for Torbjorn and my own
> testing to complete before committing.
>
> When not dealing with the special armv8.1-m.main conditional
> instructions case
> make sure it uses the default_noce_conversion_profitable_p call to determine
> whether the sequence is cost effective.
>

Ok if no regressions.

Ramana

> gcc/ChangeLog:
>
>
>         PR target/116444
>         * config/arm/arm.cc (arm_noce_conversion_profitable_p): Call
>         default_noce_conversion_profitable_p when not dealing with the
>         armv8.1-m.main conditional instructions special cases.
Torbjorn SVENSSON Oct. 7, 2024, 8:08 a.m. UTC | #2 
Hello Andre,

Compared to a run without any of the 2 patches for PR 116444, I get this 
diff:

--- base/m55hard/analysis.gcc        2024-09-18 09:07:18.879493251 +0000
+++ pr116444/m55hard/analysis.gcc        2024-10-05 11:44:05.261683071 +0000
+FAIL: gcc.target/arm/attr_thumb.c scan-assembler ite
-FAIL: gcc.target/arm/max-insns-skipped.c object-size text <= 40
-FAIL: gcc.target/arm/thumb-ifcvt-2.c scan-assembler asreq
-FAIL: gcc.target/arm/thumb-ifcvt-2.c scan-assembler lslne
+FAIL: gcc.target/arm/vseleqdf.c scan-assembler-times vseleq.f64\td[0-9]+ 1
+FAIL: gcc.target/arm/vseleqsf.c scan-assembler-times vseleq.f32\ts[0-9]+ 1
+FAIL: gcc.target/arm/vselgedf.c scan-assembler-times vselge.f64\td[0-9]+ 1
+FAIL: gcc.target/arm/vselgesf.c scan-assembler-times vselge.f32\ts[0-9]+ 1
+FAIL: gcc.target/arm/vselgtdf.c scan-assembler-times vselgt.f64\td[0-9]+ 1
+FAIL: gcc.target/arm/vselgtsf.c scan-assembler-times vselgt.f32\ts[0-9]+ 1
+FAIL: gcc.target/arm/vselledf.c scan-assembler-times vselgt.f64\td[0-9]+ 1
+FAIL: gcc.target/arm/vsellesf.c scan-assembler-times vselgt.f32\ts[0-9]+ 1
+FAIL: gcc.target/arm/vselltdf.c scan-assembler-times vselge.f64\td[0-9]+ 1
+FAIL: gcc.target/arm/vselltsf.c scan-assembler-times vselge.f32\ts[0-9]+ 1
+FAIL: gcc.target/arm/vselnedf.c scan-assembler-times vseleq.f64\td[0-9]+ 1
+FAIL: gcc.target/arm/vselnesf.c scan-assembler-times vseleq.f32\ts[0-9]+ 1
+FAIL: gcc.target/arm/vselvcdf.c scan-assembler-times vselvs.f64\td[0-9]+ 1
+FAIL: gcc.target/arm/vselvcsf.c scan-assembler-times vselvs.f32\ts[0-9]+ 1
+FAIL: gcc.target/arm/vselvsdf.c scan-assembler-times vselvs.f64\td[0-9]+ 1
+FAIL: gcc.target/arm/vselvssf.c scan-assembler-times vselvs.f32\ts[0-9]+ 1
--- base/m55soft/analysis.gcc        2024-09-18 09:07:19.199493246 +0000
+++ pr116444/m55soft/analysis.gcc        2024-10-05 11:44:07.533683037 +0000
+FAIL: gcc.target/arm/attr_thumb.c scan-assembler ite
-FAIL: gcc.target/arm/max-insns-skipped.c object-size text <= 40
-FAIL: gcc.target/arm/thumb-ifcvt-2.c scan-assembler asreq
-FAIL: gcc.target/arm/thumb-ifcvt-2.c scan-assembler lslne
+FAIL: gcc.target/arm/vseleqdf.c scan-assembler-times vseleq.f64\td[0-9]+ 1
+FAIL: gcc.target/arm/vseleqsf.c scan-assembler-times vseleq.f32\ts[0-9]+ 1
+FAIL: gcc.target/arm/vselgedf.c scan-assembler-times vselge.f64\td[0-9]+ 1
+FAIL: gcc.target/arm/vselgesf.c scan-assembler-times vselge.f32\ts[0-9]+ 1
+FAIL: gcc.target/arm/vselgtdf.c scan-assembler-times vselgt.f64\td[0-9]+ 1
+FAIL: gcc.target/arm/vselgtsf.c scan-assembler-times vselgt.f32\ts[0-9]+ 1
+FAIL: gcc.target/arm/vselledf.c scan-assembler-times vselgt.f64\td[0-9]+ 1
+FAIL: gcc.target/arm/vsellesf.c scan-assembler-times vselgt.f32\ts[0-9]+ 1
+FAIL: gcc.target/arm/vselltdf.c scan-assembler-times vselge.f64\td[0-9]+ 1
+FAIL: gcc.target/arm/vselltsf.c scan-assembler-times vselge.f32\ts[0-9]+ 1
+FAIL: gcc.target/arm/vselnedf.c scan-assembler-times vseleq.f64\td[0-9]+ 1
+FAIL: gcc.target/arm/vselnesf.c scan-assembler-times vseleq.f32\ts[0-9]+ 1
+FAIL: gcc.target/arm/vselvcdf.c scan-assembler-times vselvs.f64\td[0-9]+ 1
+FAIL: gcc.target/arm/vselvcsf.c scan-assembler-times vselvs.f32\ts[0-9]+ 1
+FAIL: gcc.target/arm/vselvsdf.c scan-assembler-times vselvs.f64\td[0-9]+ 1
+FAIL: gcc.target/arm/vselvssf.c scan-assembler-times vselvs.f32\ts[0-9]+ 1
--- base/m85hard/analysis.gcc        2024-09-18 09:07:18.035493264 +0000
+++ pr116444/m85hard/analysis.gcc        2024-10-05 11:44:04.289683085 +0000
+FAIL: gcc.target/arm/attr_thumb.c scan-assembler ite
-FAIL: gcc.target/arm/max-insns-skipped.c object-size text <= 40
-FAIL: gcc.target/arm/thumb-ifcvt-2.c scan-assembler asreq
-FAIL: gcc.target/arm/thumb-ifcvt-2.c scan-assembler lslne
+FAIL: gcc.target/arm/vseleqdf.c scan-assembler-times vseleq.f64\td[0-9]+ 1
+FAIL: gcc.target/arm/vseleqsf.c scan-assembler-times vseleq.f32\ts[0-9]+ 1
+FAIL: gcc.target/arm/vselgedf.c scan-assembler-times vselge.f64\td[0-9]+ 1
+FAIL: gcc.target/arm/vselgesf.c scan-assembler-times vselge.f32\ts[0-9]+ 1
+FAIL: gcc.target/arm/vselgtdf.c scan-assembler-times vselgt.f64\td[0-9]+ 1
+FAIL: gcc.target/arm/vselgtsf.c scan-assembler-times vselgt.f32\ts[0-9]+ 1
+FAIL: gcc.target/arm/vselledf.c scan-assembler-times vselgt.f64\td[0-9]+ 1
+FAIL: gcc.target/arm/vsellesf.c scan-assembler-times vselgt.f32\ts[0-9]+ 1
+FAIL: gcc.target/arm/vselltdf.c scan-assembler-times vselge.f64\td[0-9]+ 1
+FAIL: gcc.target/arm/vselltsf.c scan-assembler-times vselge.f32\ts[0-9]+ 1
+FAIL: gcc.target/arm/vselnedf.c scan-assembler-times vseleq.f64\td[0-9]+ 1
+FAIL: gcc.target/arm/vselnesf.c scan-assembler-times vseleq.f32\ts[0-9]+ 1
+FAIL: gcc.target/arm/vselvcdf.c scan-assembler-times vselvs.f64\td[0-9]+ 1
+FAIL: gcc.target/arm/vselvcsf.c scan-assembler-times vselvs.f32\ts[0-9]+ 1
+FAIL: gcc.target/arm/vselvsdf.c scan-assembler-times vselvs.f64\td[0-9]+ 1
+FAIL: gcc.target/arm/vselvssf.c scan-assembler-times vselvs.f32\ts[0-9]+ 1
--- base/m85soft/analysis.gcc        2024-09-18 09:07:18.351493259 +0000
+++ pr116444/m85soft/analysis.gcc        2024-10-05 11:44:04.957683075 +0000
+FAIL: gcc.target/arm/attr_thumb.c scan-assembler ite
-FAIL: gcc.target/arm/max-insns-skipped.c object-size text <= 40
-FAIL: gcc.target/arm/thumb-ifcvt-2.c scan-assembler asreq
-FAIL: gcc.target/arm/thumb-ifcvt-2.c scan-assembler lslne
+FAIL: gcc.target/arm/vseleqdf.c scan-assembler-times vseleq.f64\td[0-9]+ 1
+FAIL: gcc.target/arm/vseleqsf.c scan-assembler-times vseleq.f32\ts[0-9]+ 1
+FAIL: gcc.target/arm/vselgedf.c scan-assembler-times vselge.f64\td[0-9]+ 1
+FAIL: gcc.target/arm/vselgesf.c scan-assembler-times vselge.f32\ts[0-9]+ 1
+FAIL: gcc.target/arm/vselgtdf.c scan-assembler-times vselgt.f64\td[0-9]+ 1
+FAIL: gcc.target/arm/vselgtsf.c scan-assembler-times vselgt.f32\ts[0-9]+ 1
+FAIL: gcc.target/arm/vselledf.c scan-assembler-times vselgt.f64\td[0-9]+ 1
+FAIL: gcc.target/arm/vsellesf.c scan-assembler-times vselgt.f32\ts[0-9]+ 1
+FAIL: gcc.target/arm/vselltdf.c scan-assembler-times vselge.f64\td[0-9]+ 1
+FAIL: gcc.target/arm/vselltsf.c scan-assembler-times vselge.f32\ts[0-9]+ 1
+FAIL: gcc.target/arm/vselnedf.c scan-assembler-times vseleq.f64\td[0-9]+ 1
+FAIL: gcc.target/arm/vselnesf.c scan-assembler-times vseleq.f32\ts[0-9]+ 1
+FAIL: gcc.target/arm/vselvcdf.c scan-assembler-times vselvs.f64\td[0-9]+ 1
+FAIL: gcc.target/arm/vselvcsf.c scan-assembler-times vselvs.f32\ts[0-9]+ 1
+FAIL: gcc.target/arm/vselvsdf.c scan-assembler-times vselvs.f64\td[0-9]+ 1
+FAIL: gcc.target/arm/vselvssf.c scan-assembler-times vselvs.f32\ts[0-9]+ 1


There are 3 test cases that are fixed with these 2 commits, but there is 
also a bunch that is marked as new fails.
Looking at the test cases that fail, there are 2 different kinds of 
failures.

1. gcc.target/arm/attr_thumb.c: This test case fails due to this difference:
--- /dev/fd/63  2024-10-07 08:25:49.595309010 +0000
+++ /dev/fd/62  2024-10-07 08:25:49.575309010 +0000
@@ -33,9 +33,10 @@
         @ args = 0, pretend = 0, frame = 0
         @ frame_needed = 0, uses_anonymous_args = 0
         @ link register save eliminated.
-       cmp     r0, #0
-       ite     eq
-       moveq   r0, #5
-       movne   r0, #1
+       cbz     r0, .L3
+       movs    r0, #1
+       bx      lr
+.L3:
+       movs    r0, #5
         bx      lr
         .size   foo, .-foo
I'll leave the rest of the investigation of the reason for the failure, 
and the fix, to you Andre.


2. All other the test cases in the list above: These need to be adapted 
to the change introduced in r15-3606-g7d6c6a0d15c to have the proper arch.
I've sent a patch that should fix these "regressions" in 
https://gcc.gnu.org/pipermail/gcc-patches/2024-October/664611.html.

Let me know if you need more details on this.

Kind regards,
Torbjörn

On 2024-10-07 06:39, Ramana Radhakrishnan wrote:
> On Fri, Oct 4, 2024 at 9:25 PM Andre Vieira (lists)
> <andre.simoesdiasvieira@arm.com> wrote:
>>
>> Hi,
>>
>> The patch for 'arm: Fix missed CE optimization for armv8.1-m.main [PR
>> 116444]' introduced regressions with arm targets that used 'noce' before.
>> This is because it would approve all noce optimisations without using
>> the default cost check. Not sure why this didn't show up in my original
>> testing, I suspect you need to test this for a set of specific targets
>> like Torbjorn did, thank you for pointing these issues out to me.
>>
>> Could I ask you to rerun them with this patch? I'll try to do that
>> locally too.
>>
>> Happy to receive reviews, but I'm waiting for Torbjorn and my own
>> testing to complete before committing.
>>
>> When not dealing with the special armv8.1-m.main conditional
>> instructions case
>> make sure it uses the default_noce_conversion_profitable_p call to determine
>> whether the sequence is cost effective.
>>
> 
> Ok if no regressions.
> 
> Ramana
> 
>> gcc/ChangeLog:
>>
>>
>>          PR target/116444
>>          * config/arm/arm.cc (arm_noce_conversion_profitable_p): Call
>>          default_noce_conversion_profitable_p when not dealing with the
>>          armv8.1-m.main conditional instructions special cases.
Christophe Lyon Oct. 7, 2024, 8:37 a.m. UTC | #3 
Hi All,

FWIW, the previous patch (gcc-15-4066-g7766a2c1eb6) broke bootstrap on
arm-linux-gnueabihf. (reported via
https://linaro.atlassian.net/browse/GNU-1364)

Christophe

On Mon, 7 Oct 2024 at 10:10, Torbjorn SVENSSON
<torbjorn.svensson@foss.st.com> wrote:
>
> Hello Andre,
>
> Compared to a run without any of the 2 patches for PR 116444, I get this
> diff:
>
> --- base/m55hard/analysis.gcc        2024-09-18 09:07:18.879493251 +0000
> +++ pr116444/m55hard/analysis.gcc        2024-10-05 11:44:05.261683071 +0000
> +FAIL: gcc.target/arm/attr_thumb.c scan-assembler ite
> -FAIL: gcc.target/arm/max-insns-skipped.c object-size text <= 40
> -FAIL: gcc.target/arm/thumb-ifcvt-2.c scan-assembler asreq
> -FAIL: gcc.target/arm/thumb-ifcvt-2.c scan-assembler lslne
> +FAIL: gcc.target/arm/vseleqdf.c scan-assembler-times vseleq.f64\td[0-9]+ 1
> +FAIL: gcc.target/arm/vseleqsf.c scan-assembler-times vseleq.f32\ts[0-9]+ 1
> +FAIL: gcc.target/arm/vselgedf.c scan-assembler-times vselge.f64\td[0-9]+ 1
> +FAIL: gcc.target/arm/vselgesf.c scan-assembler-times vselge.f32\ts[0-9]+ 1
> +FAIL: gcc.target/arm/vselgtdf.c scan-assembler-times vselgt.f64\td[0-9]+ 1
> +FAIL: gcc.target/arm/vselgtsf.c scan-assembler-times vselgt.f32\ts[0-9]+ 1
> +FAIL: gcc.target/arm/vselledf.c scan-assembler-times vselgt.f64\td[0-9]+ 1
> +FAIL: gcc.target/arm/vsellesf.c scan-assembler-times vselgt.f32\ts[0-9]+ 1
> +FAIL: gcc.target/arm/vselltdf.c scan-assembler-times vselge.f64\td[0-9]+ 1
> +FAIL: gcc.target/arm/vselltsf.c scan-assembler-times vselge.f32\ts[0-9]+ 1
> +FAIL: gcc.target/arm/vselnedf.c scan-assembler-times vseleq.f64\td[0-9]+ 1
> +FAIL: gcc.target/arm/vselnesf.c scan-assembler-times vseleq.f32\ts[0-9]+ 1
> +FAIL: gcc.target/arm/vselvcdf.c scan-assembler-times vselvs.f64\td[0-9]+ 1
> +FAIL: gcc.target/arm/vselvcsf.c scan-assembler-times vselvs.f32\ts[0-9]+ 1
> +FAIL: gcc.target/arm/vselvsdf.c scan-assembler-times vselvs.f64\td[0-9]+ 1
> +FAIL: gcc.target/arm/vselvssf.c scan-assembler-times vselvs.f32\ts[0-9]+ 1
> --- base/m55soft/analysis.gcc        2024-09-18 09:07:19.199493246 +0000
> +++ pr116444/m55soft/analysis.gcc        2024-10-05 11:44:07.533683037 +0000
> +FAIL: gcc.target/arm/attr_thumb.c scan-assembler ite
> -FAIL: gcc.target/arm/max-insns-skipped.c object-size text <= 40
> -FAIL: gcc.target/arm/thumb-ifcvt-2.c scan-assembler asreq
> -FAIL: gcc.target/arm/thumb-ifcvt-2.c scan-assembler lslne
> +FAIL: gcc.target/arm/vseleqdf.c scan-assembler-times vseleq.f64\td[0-9]+ 1
> +FAIL: gcc.target/arm/vseleqsf.c scan-assembler-times vseleq.f32\ts[0-9]+ 1
> +FAIL: gcc.target/arm/vselgedf.c scan-assembler-times vselge.f64\td[0-9]+ 1
> +FAIL: gcc.target/arm/vselgesf.c scan-assembler-times vselge.f32\ts[0-9]+ 1
> +FAIL: gcc.target/arm/vselgtdf.c scan-assembler-times vselgt.f64\td[0-9]+ 1
> +FAIL: gcc.target/arm/vselgtsf.c scan-assembler-times vselgt.f32\ts[0-9]+ 1
> +FAIL: gcc.target/arm/vselledf.c scan-assembler-times vselgt.f64\td[0-9]+ 1
> +FAIL: gcc.target/arm/vsellesf.c scan-assembler-times vselgt.f32\ts[0-9]+ 1
> +FAIL: gcc.target/arm/vselltdf.c scan-assembler-times vselge.f64\td[0-9]+ 1
> +FAIL: gcc.target/arm/vselltsf.c scan-assembler-times vselge.f32\ts[0-9]+ 1
> +FAIL: gcc.target/arm/vselnedf.c scan-assembler-times vseleq.f64\td[0-9]+ 1
> +FAIL: gcc.target/arm/vselnesf.c scan-assembler-times vseleq.f32\ts[0-9]+ 1
> +FAIL: gcc.target/arm/vselvcdf.c scan-assembler-times vselvs.f64\td[0-9]+ 1
> +FAIL: gcc.target/arm/vselvcsf.c scan-assembler-times vselvs.f32\ts[0-9]+ 1
> +FAIL: gcc.target/arm/vselvsdf.c scan-assembler-times vselvs.f64\td[0-9]+ 1
> +FAIL: gcc.target/arm/vselvssf.c scan-assembler-times vselvs.f32\ts[0-9]+ 1
> --- base/m85hard/analysis.gcc        2024-09-18 09:07:18.035493264 +0000
> +++ pr116444/m85hard/analysis.gcc        2024-10-05 11:44:04.289683085 +0000
> +FAIL: gcc.target/arm/attr_thumb.c scan-assembler ite
> -FAIL: gcc.target/arm/max-insns-skipped.c object-size text <= 40
> -FAIL: gcc.target/arm/thumb-ifcvt-2.c scan-assembler asreq
> -FAIL: gcc.target/arm/thumb-ifcvt-2.c scan-assembler lslne
> +FAIL: gcc.target/arm/vseleqdf.c scan-assembler-times vseleq.f64\td[0-9]+ 1
> +FAIL: gcc.target/arm/vseleqsf.c scan-assembler-times vseleq.f32\ts[0-9]+ 1
> +FAIL: gcc.target/arm/vselgedf.c scan-assembler-times vselge.f64\td[0-9]+ 1
> +FAIL: gcc.target/arm/vselgesf.c scan-assembler-times vselge.f32\ts[0-9]+ 1
> +FAIL: gcc.target/arm/vselgtdf.c scan-assembler-times vselgt.f64\td[0-9]+ 1
> +FAIL: gcc.target/arm/vselgtsf.c scan-assembler-times vselgt.f32\ts[0-9]+ 1
> +FAIL: gcc.target/arm/vselledf.c scan-assembler-times vselgt.f64\td[0-9]+ 1
> +FAIL: gcc.target/arm/vsellesf.c scan-assembler-times vselgt.f32\ts[0-9]+ 1
> +FAIL: gcc.target/arm/vselltdf.c scan-assembler-times vselge.f64\td[0-9]+ 1
> +FAIL: gcc.target/arm/vselltsf.c scan-assembler-times vselge.f32\ts[0-9]+ 1
> +FAIL: gcc.target/arm/vselnedf.c scan-assembler-times vseleq.f64\td[0-9]+ 1
> +FAIL: gcc.target/arm/vselnesf.c scan-assembler-times vseleq.f32\ts[0-9]+ 1
> +FAIL: gcc.target/arm/vselvcdf.c scan-assembler-times vselvs.f64\td[0-9]+ 1
> +FAIL: gcc.target/arm/vselvcsf.c scan-assembler-times vselvs.f32\ts[0-9]+ 1
> +FAIL: gcc.target/arm/vselvsdf.c scan-assembler-times vselvs.f64\td[0-9]+ 1
> +FAIL: gcc.target/arm/vselvssf.c scan-assembler-times vselvs.f32\ts[0-9]+ 1
> --- base/m85soft/analysis.gcc        2024-09-18 09:07:18.351493259 +0000
> +++ pr116444/m85soft/analysis.gcc        2024-10-05 11:44:04.957683075 +0000
> +FAIL: gcc.target/arm/attr_thumb.c scan-assembler ite
> -FAIL: gcc.target/arm/max-insns-skipped.c object-size text <= 40
> -FAIL: gcc.target/arm/thumb-ifcvt-2.c scan-assembler asreq
> -FAIL: gcc.target/arm/thumb-ifcvt-2.c scan-assembler lslne
> +FAIL: gcc.target/arm/vseleqdf.c scan-assembler-times vseleq.f64\td[0-9]+ 1
> +FAIL: gcc.target/arm/vseleqsf.c scan-assembler-times vseleq.f32\ts[0-9]+ 1
> +FAIL: gcc.target/arm/vselgedf.c scan-assembler-times vselge.f64\td[0-9]+ 1
> +FAIL: gcc.target/arm/vselgesf.c scan-assembler-times vselge.f32\ts[0-9]+ 1
> +FAIL: gcc.target/arm/vselgtdf.c scan-assembler-times vselgt.f64\td[0-9]+ 1
> +FAIL: gcc.target/arm/vselgtsf.c scan-assembler-times vselgt.f32\ts[0-9]+ 1
> +FAIL: gcc.target/arm/vselledf.c scan-assembler-times vselgt.f64\td[0-9]+ 1
> +FAIL: gcc.target/arm/vsellesf.c scan-assembler-times vselgt.f32\ts[0-9]+ 1
> +FAIL: gcc.target/arm/vselltdf.c scan-assembler-times vselge.f64\td[0-9]+ 1
> +FAIL: gcc.target/arm/vselltsf.c scan-assembler-times vselge.f32\ts[0-9]+ 1
> +FAIL: gcc.target/arm/vselnedf.c scan-assembler-times vseleq.f64\td[0-9]+ 1
> +FAIL: gcc.target/arm/vselnesf.c scan-assembler-times vseleq.f32\ts[0-9]+ 1
> +FAIL: gcc.target/arm/vselvcdf.c scan-assembler-times vselvs.f64\td[0-9]+ 1
> +FAIL: gcc.target/arm/vselvcsf.c scan-assembler-times vselvs.f32\ts[0-9]+ 1
> +FAIL: gcc.target/arm/vselvsdf.c scan-assembler-times vselvs.f64\td[0-9]+ 1
> +FAIL: gcc.target/arm/vselvssf.c scan-assembler-times vselvs.f32\ts[0-9]+ 1
>
>
> There are 3 test cases that are fixed with these 2 commits, but there is
> also a bunch that is marked as new fails.
> Looking at the test cases that fail, there are 2 different kinds of
> failures.
>
> 1. gcc.target/arm/attr_thumb.c: This test case fails due to this difference:
> --- /dev/fd/63  2024-10-07 08:25:49.595309010 +0000
> +++ /dev/fd/62  2024-10-07 08:25:49.575309010 +0000
> @@ -33,9 +33,10 @@
>          @ args = 0, pretend = 0, frame = 0
>          @ frame_needed = 0, uses_anonymous_args = 0
>          @ link register save eliminated.
> -       cmp     r0, #0
> -       ite     eq
> -       moveq   r0, #5
> -       movne   r0, #1
> +       cbz     r0, .L3
> +       movs    r0, #1
> +       bx      lr
> +.L3:
> +       movs    r0, #5
>          bx      lr
>          .size   foo, .-foo
> I'll leave the rest of the investigation of the reason for the failure,
> and the fix, to you Andre.
>
>
> 2. All other the test cases in the list above: These need to be adapted
> to the change introduced in r15-3606-g7d6c6a0d15c to have the proper arch.
> I've sent a patch that should fix these "regressions" in
> https://gcc.gnu.org/pipermail/gcc-patches/2024-October/664611.html.
>
> Let me know if you need more details on this.
>
> Kind regards,
> Torbjörn
>
> On 2024-10-07 06:39, Ramana Radhakrishnan wrote:
> > On Fri, Oct 4, 2024 at 9:25 PM Andre Vieira (lists)
> > <andre.simoesdiasvieira@arm.com> wrote:
> >>
> >> Hi,
> >>
> >> The patch for 'arm: Fix missed CE optimization for armv8.1-m.main [PR
> >> 116444]' introduced regressions with arm targets that used 'noce' before.
> >> This is because it would approve all noce optimisations without using
> >> the default cost check. Not sure why this didn't show up in my original
> >> testing, I suspect you need to test this for a set of specific targets
> >> like Torbjorn did, thank you for pointing these issues out to me.
> >>
> >> Could I ask you to rerun them with this patch? I'll try to do that
> >> locally too.
> >>
> >> Happy to receive reviews, but I'm waiting for Torbjorn and my own
> >> testing to complete before committing.
> >>
> >> When not dealing with the special armv8.1-m.main conditional
> >> instructions case
> >> make sure it uses the default_noce_conversion_profitable_p call to determine
> >> whether the sequence is cost effective.
> >>
> >
> > Ok if no regressions.
> >
> > Ramana
> >
> >> gcc/ChangeLog:
> >>
> >>
> >>          PR target/116444
> >>          * config/arm/arm.cc (arm_noce_conversion_profitable_p): Call
> >>          default_noce_conversion_profitable_p when not dealing with the
> >>          armv8.1-m.main conditional instructions special cases.
>
Andre Vieira (lists) Oct. 7, 2024, 8:53 a.m. UTC | #4 
Hi Torbjorn,

On 07/10/2024 09:08, Torbjorn SVENSSON wrote:


> 
> There are 3 test cases that are fixed with these 2 commits, but there is 
> also a bunch that is marked as new fails.
> Looking at the test cases that fail, there are 2 different kinds of 
> failures.
> 
> 1. gcc.target/arm/attr_thumb.c: This test case fails due to this 
> difference:
> --- /dev/fd/63  2024-10-07 08:25:49.595309010 +0000
> +++ /dev/fd/62  2024-10-07 08:25:49.575309010 +0000
> @@ -33,9 +33,10 @@
>          @ args = 0, pretend = 0, frame = 0
>          @ frame_needed = 0, uses_anonymous_args = 0
>          @ link register save eliminated.
> -       cmp     r0, #0
> -       ite     eq
> -       moveq   r0, #5
> -       movne   r0, #1
> +       cbz     r0, .L3
> +       movs    r0, #1
> +       bx      lr
> +.L3:
> +       movs    r0, #5
>          bx      lr
>          .size   foo, .-foo
> I'll leave the rest of the investigation of the reason for the failure, 
> and the fix, to you Andre.

I think this test was meant to check __attribute__((thumb)) worked by 
switching to thumb, forcing a specific type of codegen, which no longer 
holds for armv8.1-m, so this is a testism that needs some creative 
thinking, probably best to skip if armv8.1-m.


> 
> 2. All other the test cases in the list above: These need to be adapted 
> to the change introduced in r15-3606-g7d6c6a0d15c to have the proper arch.
> I've sent a patch that should fix these "regressions" in https:// 
> gcc.gnu.org/pipermail/gcc-patches/2024-October/664611.html.
>
I  presume you are using -march=armv8.1-m.main+mve.fp+fp.dp for these 
rather than -mcpu? If I do:
RUNTESTFLAGS="--target_board=<...>/-mcpu=cortex-m55/-mfloat-abi=hard 
arm.exp=vseleqdf.c" then it works just fine for me as the -mcpu=unset 
does it work, but the -march=armv8.1-m.main+mve.fp+fp.dp does fail. I'll 
talk to Richard E about this one.

Thanks for helping with the testing I'll send a patch with the testism 
fixes up later.

I am however quite confident that these are both testisms. @Christophe: 
Any chance you can run the second patch through the bootstrap CI for 
arm-none-linux-gnueabihf ? Might end up committing the 2nd patch first 
if it helps fix that?
Torbjorn SVENSSON Oct. 7, 2024, 9:02 a.m. UTC | #5 
On 2024-10-07 10:53, Andre Vieira (lists) wrote:
> Hi Torbjorn,
> 
> On 07/10/2024 09:08, Torbjorn SVENSSON wrote:
> 
> 
>>
>> There are 3 test cases that are fixed with these 2 commits, but there 
>> is also a bunch that is marked as new fails.
>> Looking at the test cases that fail, there are 2 different kinds of 
>> failures.
>>
>> 1. gcc.target/arm/attr_thumb.c: This test case fails due to this 
>> difference:
>> --- /dev/fd/63  2024-10-07 08:25:49.595309010 +0000
>> +++ /dev/fd/62  2024-10-07 08:25:49.575309010 +0000
>> @@ -33,9 +33,10 @@
>>          @ args = 0, pretend = 0, frame = 0
>>          @ frame_needed = 0, uses_anonymous_args = 0
>>          @ link register save eliminated.
>> -       cmp     r0, #0
>> -       ite     eq
>> -       moveq   r0, #5
>> -       movne   r0, #1
>> +       cbz     r0, .L3
>> +       movs    r0, #1
>> +       bx      lr
>> +.L3:
>> +       movs    r0, #5
>>          bx      lr
>>          .size   foo, .-foo
>> I'll leave the rest of the investigation of the reason for the 
>> failure, and the fix, to you Andre.
> 
> I think this test was meant to check __attribute__((thumb)) worked by 
> switching to thumb, forcing a specific type of codegen, which no longer 
> holds for armv8.1-m, so this is a testism that needs some creative 
> thinking, probably best to skip if armv8.1-m.
> 
> 
>>
>> 2. All other the test cases in the list above: These need to be 
>> adapted to the change introduced in r15-3606-g7d6c6a0d15c to have the 
>> proper arch.
>> I've sent a patch that should fix these "regressions" in https:// 
>> gcc.gnu.org/pipermail/gcc-patches/2024-October/664611.html.
>>
> I  presume you are using -march=armv8.1-m.main+mve.fp+fp.dp for these 
> rather than -mcpu? If I do:
> RUNTESTFLAGS="--target_board=<...>/-mcpu=cortex-m55/-mfloat-abi=hard 
> arm.exp=vseleqdf.c" then it works just fine for me as the -mcpu=unset 
> does it work, but the -march=armv8.1-m.main+mve.fp+fp.dp does fail. I'll 
> talk to Richard E about this one.

For these results, I did as before. This is the command line that was 
used for gcc.target/arm/vseleqdf.c:
.../bin/arm-none-eabi-gcc  .../gcc/testsuite/gcc.target/arm/vseleqdf.c 
-mthumb -march=armv8.1-m.main+mve+pacbti -mcpu=cortex-m85 
-mfloat-abi=hard -mfpu=fpv5-d16   -fdiagnostics-plain-output  -O2 
-mcpu=cortex-a57 -mfpu=fp-armv8 -mfloat-abi=softfp -ffat-lto-objects 
-fno-ident -S     -o vseleqdf.s

> 
> Thanks for helping with the testing I'll send a patch with the testism 
> fixes up later.
> 
> I am however quite confident that these are both testisms. @Christophe: 
> Any chance you can run the second patch through the bootstrap CI for 
> arm-none-linux-gnueabihf ? Might end up committing the 2nd patch first 
> if it helps fix that?

I think the reason for the bootstrap failure is simply:
- the 2nd argument to arm_noce_conversion_profitable_p is never 
referenced in the function.
- the variable "set" in arm_is_v81m_cond_insn might not be defined 
before the if-statement (in the case the while loop is never executed).

Kind regards,
Torbjörn
Christophe Lyon Oct. 7, 2024, 9:15 a.m. UTC | #6 
On Mon, 7 Oct 2024 at 11:04, Torbjorn SVENSSON
<torbjorn.svensson@foss.st.com> wrote:
>
>
>
> On 2024-10-07 10:53, Andre Vieira (lists) wrote:
> > Hi Torbjorn,
> >
> > On 07/10/2024 09:08, Torbjorn SVENSSON wrote:
> >
> >
> >>
> >> There are 3 test cases that are fixed with these 2 commits, but there
> >> is also a bunch that is marked as new fails.
> >> Looking at the test cases that fail, there are 2 different kinds of
> >> failures.
> >>
> >> 1. gcc.target/arm/attr_thumb.c: This test case fails due to this
> >> difference:
> >> --- /dev/fd/63  2024-10-07 08:25:49.595309010 +0000
> >> +++ /dev/fd/62  2024-10-07 08:25:49.575309010 +0000
> >> @@ -33,9 +33,10 @@
> >>          @ args = 0, pretend = 0, frame = 0
> >>          @ frame_needed = 0, uses_anonymous_args = 0
> >>          @ link register save eliminated.
> >> -       cmp     r0, #0
> >> -       ite     eq
> >> -       moveq   r0, #5
> >> -       movne   r0, #1
> >> +       cbz     r0, .L3
> >> +       movs    r0, #1
> >> +       bx      lr
> >> +.L3:
> >> +       movs    r0, #5
> >>          bx      lr
> >>          .size   foo, .-foo
> >> I'll leave the rest of the investigation of the reason for the
> >> failure, and the fix, to you Andre.
> >
> > I think this test was meant to check __attribute__((thumb)) worked by
> > switching to thumb, forcing a specific type of codegen, which no longer
> > holds for armv8.1-m, so this is a testism that needs some creative
> > thinking, probably best to skip if armv8.1-m.
> >
> >
> >>
> >> 2. All other the test cases in the list above: These need to be
> >> adapted to the change introduced in r15-3606-g7d6c6a0d15c to have the
> >> proper arch.
> >> I've sent a patch that should fix these "regressions" in https://
> >> gcc.gnu.org/pipermail/gcc-patches/2024-October/664611.html.
> >>
> > I  presume you are using -march=armv8.1-m.main+mve.fp+fp.dp for these
> > rather than -mcpu? If I do:
> > RUNTESTFLAGS="--target_board=<...>/-mcpu=cortex-m55/-mfloat-abi=hard
> > arm.exp=vseleqdf.c" then it works just fine for me as the -mcpu=unset
> > does it work, but the -march=armv8.1-m.main+mve.fp+fp.dp does fail. I'll
> > talk to Richard E about this one.
>
> For these results, I did as before. This is the command line that was
> used for gcc.target/arm/vseleqdf.c:
> .../bin/arm-none-eabi-gcc  .../gcc/testsuite/gcc.target/arm/vseleqdf.c
> -mthumb -march=armv8.1-m.main+mve+pacbti -mcpu=cortex-m85
> -mfloat-abi=hard -mfpu=fpv5-d16   -fdiagnostics-plain-output  -O2
> -mcpu=cortex-a57 -mfpu=fp-armv8 -mfloat-abi=softfp -ffat-lto-objects
> -fno-ident -S     -o vseleqdf.s
>
> >
> > Thanks for helping with the testing I'll send a patch with the testism
> > fixes up later.
> >
> > I am however quite confident that these are both testisms. @Christophe:
> > Any chance you can run the second patch through the bootstrap CI for
> > arm-none-linux-gnueabihf ? Might end up committing the 2nd patch first
> > if it helps fix that?
>
> I think the reason for the bootstrap failure is simply:
> - the 2nd argument to arm_noce_conversion_profitable_p is never
> referenced in the function.
> - the variable "set" in arm_is_v81m_cond_insn might not be defined
> before the if-statement (in the case the while loop is never executed).
>

Right: I was mainly mentioning the bootstrap problem in case it takes
time to come to an agreement for this patch. (bootstrap is easy to
fix)

(@Andre I did manually start a precommit CI bootstrap build for this
patch, but it fails to apply because we keep the baseline as before
bootstrap is broken)

Thanks,

Christophe


> Kind regards,
> Torbjörn
>
Andre Vieira (lists) Oct. 7, 2024, 10:29 a.m. UTC | #7 
On 07/10/2024 10:15, Christophe Lyon wrote:
> On Mon, 7 Oct 2024 at 11:04, Torbjorn SVENSSON
> <torbjorn.svensson@foss.st.com> wrote:
>>
>>
>>
>> On 2024-10-07 10:53, Andre Vieira (lists) wrote:
>>> Hi Torbjorn,
>>>
>>>>
>>>> 2. All other the test cases in the list above: These need to be
>>>> adapted to the change introduced in r15-3606-g7d6c6a0d15c to have the
>>>> proper arch.
>>>> I've sent a patch that should fix these "regressions" in https://
>>>> gcc.gnu.org/pipermail/gcc-patches/2024-October/664611.html.
>>>>
>>> I  presume you are using -march=armv8.1-m.main+mve.fp+fp.dp for these
>>> rather than -mcpu? If I do:
>>> RUNTESTFLAGS="--target_board=<...>/-mcpu=cortex-m55/-mfloat-abi=hard
>>> arm.exp=vseleqdf.c" then it works just fine for me as the -mcpu=unset
>>> does it work, but the -march=armv8.1-m.main+mve.fp+fp.dp does fail. I'll
>>> talk to Richard E about this one.
>>
>> For these results, I did as before. This is the command line that was
>> used for gcc.target/arm/vseleqdf.c:
>> .../bin/arm-none-eabi-gcc  .../gcc/testsuite/gcc.target/arm/vseleqdf.c
>> -mthumb -march=armv8.1-m.main+mve+pacbti -mcpu=cortex-m85
>> -mfloat-abi=hard -mfpu=fpv5-d16   -fdiagnostics-plain-output  -O2
>> -mcpu=cortex-a57 -mfpu=fp-armv8 -mfloat-abi=softfp -ffat-lto-objects
>> -fno-ident -S     -o vseleqdf.s
>>
>>>

@Torbjorn:
Where are all these options coming from though? There are some 
'conflicts' here. If you are passing all of these to RUNTESTFLAGS then 
... please don't.
For instance:
-march=armv8.1-m.main+mve+pacbti -mcpu=cortex-m85
These are contradictory:
Use either -march or -mcpu.
-mcpu=cortex-m85 is equivalent to:
-march=armv8.1-m.main+pacbti+mve.fp+fp.dp -mtune=cortex-m85

Your -march only enables integer mve it also doesn't enable any FP, 
cortex-m85 only seems to allow for double precision FP, we dont' seem to 
have a single FP configuration in GCC.

I also see a -mfpu=fpv5-d16, if you are passing that, then please don't. 
You should be passing -mfpu=auto if anything when compiling for MVE 
cores as there is no -mfpu= option that enables MVE and we don't 
recommend using both architecture extensions and -mfpu options.

Having said all this, I do now realize vseleq.f64 is available for 
armv8.1-m.main, so we should also enable that. I'm having a look!

> Right: I was mainly mentioning the bootstrap problem in case it takes
> time to come to an agreement for this patch. (bootstrap is easy to
> fix)
> 
> (@Andre I did manually start a precommit CI bootstrap build for this
> patch, but it fails to apply because we keep the baseline as before
> bootstrap is broken)

Yeah, hadn't had the chance to look at the fail log. But I'll commit the 
fix as obvious today, regardless of the issues above.
diff mbox series

Patch

diff --git a/gcc/config/arm/arm.cc b/gcc/config/arm/arm.cc
index 077c80df4482d168d9694795be68c2eeb8f304d9..fd437f428781673e1d44498d31a47f174e0f57fa 100644
--- a/gcc/config/arm/arm.cc
+++ b/gcc/config/arm/arm.cc
@@ -36168,7 +36168,7 @@  arm_noce_conversion_profitable_p (rtx_insn *seq, struct noce_if_info *if_info)
 {
   if (!TARGET_COND_ARITH
       || reload_completed)
-    return true;
+    return default_noce_conversion_profitable_p (seq, if_info);
 
   if (arm_is_v81m_cond_insn (seq))
     return true;