* [PATCH] rtlanal: Fix set_noop_p for volatile loads or stores [PR114768] @ 2024-04-19 6:24 Jakub Jelinek 2024-04-19 6:33 ` Richard Biener 2024-04-19 10:23 ` Thomas Schwinge 0 siblings, 2 replies; 5+ messages in thread From: Jakub Jelinek @ 2024-04-19 6:24 UTC (permalink / raw) To: Richard Biener, Jeff Law, Eric Botcazou; +Cc: gcc-patches Hi! On the following testcase, combine propagates the mem/v load into mem store with the same address and then removes it, because noop_move_p says it is a no-op move. If it was the other way around, i.e. mem/v store and mem load, or both would be mem/v, it would be kept. The problem is that rtx_equal_p never checks any kind of flags on the rtxes (and I think it would be quite dangerous to change it at this point), and set_noop_p checks side_effects_p on just one of the operands, not both. In the MEM <- MEM set, it only checks it on the destination, in store to ZERO_EXTRACT only checks it on the source. The following patch adds the missing side_effects_p checks. Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? 2024-04-19 Jakub Jelinek <jakub@redhat.com> PR rtl-optimization/114768 * rtlanal.cc (set_noop_p): Don't return true for MEM <- MEM sets if src has side-effects or for stores into ZERO_EXTRACT if ZERO_EXTRACT operand has side-effects. * gcc.dg/pr114768.c: New test. --- gcc/rtlanal.cc.jj 2024-02-24 12:45:28.674249100 +0100 +++ gcc/rtlanal.cc 2024-04-18 15:09:55.199499083 +0200 @@ -1637,12 +1637,15 @@ set_noop_p (const_rtx set) return true; if (MEM_P (dst) && MEM_P (src)) - return rtx_equal_p (dst, src) && !side_effects_p (dst); + return (rtx_equal_p (dst, src) + && !side_effects_p (dst) + && !side_effects_p (src)); if (GET_CODE (dst) == ZERO_EXTRACT) - return rtx_equal_p (XEXP (dst, 0), src) - && !BITS_BIG_ENDIAN && XEXP (dst, 2) == const0_rtx - && !side_effects_p (src); + return (rtx_equal_p (XEXP (dst, 0), src) + && !BITS_BIG_ENDIAN && XEXP (dst, 2) == const0_rtx + && !side_effects_p (src) + && !side_effects_p (XEXP (dst, 0))); if (GET_CODE (dst) == STRICT_LOW_PART) dst = XEXP (dst, 0); --- gcc/testsuite/gcc.dg/pr114768.c.jj 2024-04-18 15:37:49.139433678 +0200 +++ gcc/testsuite/gcc.dg/pr114768.c 2024-04-18 15:43:30.389730365 +0200 @@ -0,0 +1,10 @@ +/* PR rtl-optimization/114768 */ +/* { dg-do compile } */ +/* { dg-options "-O2 -fdump-rtl-final" } */ +/* { dg-final { scan-rtl-dump "\\\(mem/v:" "final" { target { ! { nvptx*-*-* } } } } } */ + +void +foo (int *p) +{ + *p = *(volatile int *) p; +} Jakub ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] rtlanal: Fix set_noop_p for volatile loads or stores [PR114768] 2024-04-19 6:24 [PATCH] rtlanal: Fix set_noop_p for volatile loads or stores [PR114768] Jakub Jelinek @ 2024-04-19 6:33 ` Richard Biener 2024-04-19 10:23 ` Thomas Schwinge 1 sibling, 0 replies; 5+ messages in thread From: Richard Biener @ 2024-04-19 6:33 UTC (permalink / raw) To: Jakub Jelinek; +Cc: Jeff Law, Eric Botcazou, gcc-patches On Fri, 19 Apr 2024, Jakub Jelinek wrote: > Hi! > > On the following testcase, combine propagates the mem/v load into mem store > with the same address and then removes it, because noop_move_p says it is a > no-op move. If it was the other way around, i.e. mem/v store and mem load, > or both would be mem/v, it would be kept. > The problem is that rtx_equal_p never checks any kind of flags on the rtxes > (and I think it would be quite dangerous to change it at this point), and > set_noop_p checks side_effects_p on just one of the operands, not both. > In the MEM <- MEM set, it only checks it on the destination, in > store to ZERO_EXTRACT only checks it on the source. > > The following patch adds the missing side_effects_p checks. > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? OK. Thanks, Richard. > 2024-04-19 Jakub Jelinek <jakub@redhat.com> > > PR rtl-optimization/114768 > * rtlanal.cc (set_noop_p): Don't return true for MEM <- MEM > sets if src has side-effects or for stores into ZERO_EXTRACT > if ZERO_EXTRACT operand has side-effects. > > * gcc.dg/pr114768.c: New test. > > --- gcc/rtlanal.cc.jj 2024-02-24 12:45:28.674249100 +0100 > +++ gcc/rtlanal.cc 2024-04-18 15:09:55.199499083 +0200 > @@ -1637,12 +1637,15 @@ set_noop_p (const_rtx set) > return true; > > if (MEM_P (dst) && MEM_P (src)) > - return rtx_equal_p (dst, src) && !side_effects_p (dst); > + return (rtx_equal_p (dst, src) > + && !side_effects_p (dst) > + && !side_effects_p (src)); > > if (GET_CODE (dst) == ZERO_EXTRACT) > - return rtx_equal_p (XEXP (dst, 0), src) > - && !BITS_BIG_ENDIAN && XEXP (dst, 2) == const0_rtx > - && !side_effects_p (src); > + return (rtx_equal_p (XEXP (dst, 0), src) > + && !BITS_BIG_ENDIAN && XEXP (dst, 2) == const0_rtx > + && !side_effects_p (src) > + && !side_effects_p (XEXP (dst, 0))); > > if (GET_CODE (dst) == STRICT_LOW_PART) > dst = XEXP (dst, 0); > --- gcc/testsuite/gcc.dg/pr114768.c.jj 2024-04-18 15:37:49.139433678 +0200 > +++ gcc/testsuite/gcc.dg/pr114768.c 2024-04-18 15:43:30.389730365 +0200 > @@ -0,0 +1,10 @@ > +/* PR rtl-optimization/114768 */ > +/* { dg-do compile } */ > +/* { dg-options "-O2 -fdump-rtl-final" } */ > +/* { dg-final { scan-rtl-dump "\\\(mem/v:" "final" { target { ! { nvptx*-*-* } } } } } */ > + > +void > +foo (int *p) > +{ > + *p = *(volatile int *) p; > +} > > Jakub > > -- Richard Biener <rguenther@suse.de> SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg, Germany; GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg) ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] rtlanal: Fix set_noop_p for volatile loads or stores [PR114768] 2024-04-19 6:24 [PATCH] rtlanal: Fix set_noop_p for volatile loads or stores [PR114768] Jakub Jelinek 2024-04-19 6:33 ` Richard Biener @ 2024-04-19 10:23 ` Thomas Schwinge 2024-04-19 10:30 ` Jakub Jelinek 1 sibling, 1 reply; 5+ messages in thread From: Thomas Schwinge @ 2024-04-19 10:23 UTC (permalink / raw) To: Jakub Jelinek Cc: gcc-patches, Richard Biener, Jeff Law, Eric Botcazou, Tom de Vries Hi Jakub! On 2024-04-19T08:24:03+0200, Jakub Jelinek <jakub@redhat.com> wrote: > --- gcc/testsuite/gcc.dg/pr114768.c.jj 2024-04-18 15:37:49.139433678 +0200 > +++ gcc/testsuite/gcc.dg/pr114768.c 2024-04-18 15:43:30.389730365 +0200 > @@ -0,0 +1,10 @@ > +/* PR rtl-optimization/114768 */ > +/* { dg-do compile } */ > +/* { dg-options "-O2 -fdump-rtl-final" } */ > +/* { dg-final { scan-rtl-dump "\\\(mem/v:" "final" { target { ! { nvptx*-*-* } } } } } */ > + > +void > +foo (int *p) > +{ > + *p = *(volatile int *) p; > +} Why exclude nvptx target here? As far as I can see, it does behave in the exactly same way as expected; see 'diff' of before vs. after the 'gcc/rtlanal.cc' code changes: PASS: gcc.dg/pr114768.c (test for excess errors) [-FAIL:-]{+PASS:+} gcc.dg/pr114768.c scan-rtl-dump final "\\(mem/v:" --- 0/pr114768.c.347r.final 2024-04-19 11:34:34.577037596 +0200 +++ ./pr114768.c.347r.final 2024-04-19 12:08:00.118312524 +0200 @@ -13,15 +13,27 @@ ;; entry block defs 1 [%stack] 2 [%frame] 3 [%args] ;; exit block uses 1 [%stack] 2 [%frame] ;; regs ever live -;; ref usage r1={1d,2u} r2={1d,2u} r3={1d,1u} -;; total ref usage 8{3d,5u,0e} in 1{1 regular + 0 call} insns. +;; ref usage r1={1d,3u} r2={1d,3u} r3={1d,2u} r22={1d,1u} r23={1d,2u} +;; total ref usage 16{5d,11u,0e} in 4{4 regular + 0 call} insns. (note 1 0 4 NOTE_INSN_DELETED) (note 4 1 2 2 [bb 2] NOTE_INSN_BASIC_BLOCK) -(note 2 4 3 2 NOTE_INSN_DELETED) +(insn 2 4 3 2 (set (reg/v/f:DI 23 [ p ]) + (unspec:DI [ + (const_int 0 [0]) + ] UNSPEC_ARG_REG)) "source-gcc/gcc/testsuite/gcc.dg/pr114768.c":8:1 14 {load_arg_regdi} + (nil)) (note 3 2 6 2 NOTE_INSN_FUNCTION_BEG) -(note 6 3 10 2 NOTE_INSN_DELETED) -(note 10 6 11 2 NOTE_INSN_EPILOGUE_BEG) -(jump_insn 11 10 12 2 (return) "source-gcc/gcc/testsuite/gcc.dg/pr114768.c":10:1 289 {return} +(insn 6 3 7 2 (set (reg:SI 22 [ _1 ]) + (mem/v:SI (reg/v/f:DI 23 [ p ]) [1 MEM[(volatile int *)p_3(D)]+0 S4 A32])) "source-gcc/gcc/testsuite/gcc.dg/pr114768.c":9:8 6 {*movsi_insn} + (nil)) +(insn 7 6 10 2 (set (mem:SI (reg/v/f:DI 23 [ p ]) [1 *p_3(D)+0 S4 A32]) + (reg:SI 22 [ _1 ])) "source-gcc/gcc/testsuite/gcc.dg/pr114768.c":9:6 6 {*movsi_insn} + (expr_list:REG_DEAD (reg/v/f:DI 23 [ p ]) + (expr_list:REG_DEAD (reg:SI 22 [ _1 ]) + (nil)))) +(note 10 7 13 2 NOTE_INSN_EPILOGUE_BEG) +(note 13 10 11 3 [bb 3] NOTE_INSN_BASIC_BLOCK) +(jump_insn 11 13 12 3 (return) "source-gcc/gcc/testsuite/gcc.dg/pr114768.c":10:1 289 {return} (nil) -> return) (barrier 12 11 0) --- 0/pr114768.s 2024-04-19 11:34:34.577037596 +0200 +++ ./pr114768.s 2024-04-19 12:08:00.118312524 +0200 @@ -13,5 +13,10 @@ { .reg.u64 %ar0; ld.param.u64 %ar0, [%in_ar0]; + .reg.u32 %r22; + .reg.u64 %r23; + mov.u64 %r23, %ar0; + ld.u32 %r22, [%r23]; + st.u32 [%r23], %r22; ret; } Grüße Thomas ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] rtlanal: Fix set_noop_p for volatile loads or stores [PR114768] 2024-04-19 10:23 ` Thomas Schwinge @ 2024-04-19 10:30 ` Jakub Jelinek 2024-04-19 10:39 ` Enable 'gcc.dg/pr114768.c' for nvptx target [PR114768] (was: [PATCH] rtlanal: Fix set_noop_p for volatile loads or stores [PR114768]) Thomas Schwinge 0 siblings, 1 reply; 5+ messages in thread From: Jakub Jelinek @ 2024-04-19 10:30 UTC (permalink / raw) To: Thomas Schwinge Cc: gcc-patches, Richard Biener, Jeff Law, Eric Botcazou, Tom de Vries On Fri, Apr 19, 2024 at 12:23:03PM +0200, Thomas Schwinge wrote: > On 2024-04-19T08:24:03+0200, Jakub Jelinek <jakub@redhat.com> wrote: > > --- gcc/testsuite/gcc.dg/pr114768.c.jj 2024-04-18 15:37:49.139433678 +0200 > > +++ gcc/testsuite/gcc.dg/pr114768.c 2024-04-18 15:43:30.389730365 +0200 > > @@ -0,0 +1,10 @@ > > +/* PR rtl-optimization/114768 */ > > +/* { dg-do compile } */ > > +/* { dg-options "-O2 -fdump-rtl-final" } */ > > +/* { dg-final { scan-rtl-dump "\\\(mem/v:" "final" { target { ! { nvptx*-*-* } } } } } */ > > + > > +void > > +foo (int *p) > > +{ > > + *p = *(volatile int *) p; > > +} > > Why exclude nvptx target here? As far as I can see, it does behave in > the exactly same way as expected; see 'diff' of before vs. after the > 'gcc/rtlanal.cc' code changes: I wasn't sure if the non-RA targets (for which we don't have an effective target) even have final dump. If they do as you show, then guess the target guard can go. Jakub ^ permalink raw reply [flat|nested] 5+ messages in thread
* Enable 'gcc.dg/pr114768.c' for nvptx target [PR114768] (was: [PATCH] rtlanal: Fix set_noop_p for volatile loads or stores [PR114768]) 2024-04-19 10:30 ` Jakub Jelinek @ 2024-04-19 10:39 ` Thomas Schwinge 0 siblings, 0 replies; 5+ messages in thread From: Thomas Schwinge @ 2024-04-19 10:39 UTC (permalink / raw) To: Jakub Jelinek, gcc-patches Cc: Richard Biener, Jeff Law, Eric Botcazou, Tom de Vries [-- Attachment #1: Type: text/plain, Size: 1259 bytes --] Hi! On 2024-04-19T12:30:25+0200, Jakub Jelinek <jakub@redhat.com> wrote: > On Fri, Apr 19, 2024 at 12:23:03PM +0200, Thomas Schwinge wrote: >> On 2024-04-19T08:24:03+0200, Jakub Jelinek <jakub@redhat.com> wrote: >> > --- gcc/testsuite/gcc.dg/pr114768.c.jj 2024-04-18 15:37:49.139433678 +0200 >> > +++ gcc/testsuite/gcc.dg/pr114768.c 2024-04-18 15:43:30.389730365 +0200 >> > @@ -0,0 +1,10 @@ >> > +/* PR rtl-optimization/114768 */ >> > +/* { dg-do compile } */ >> > +/* { dg-options "-O2 -fdump-rtl-final" } */ >> > +/* { dg-final { scan-rtl-dump "\\\(mem/v:" "final" { target { ! { nvptx*-*-* } } } } } */ >> > + >> > +void >> > +foo (int *p) >> > +{ >> > + *p = *(volatile int *) p; >> > +} >> >> Why exclude nvptx target here? As far as I can see, it does behave in >> the exactly same way as expected; see 'diff' of before vs. after the >> 'gcc/rtlanal.cc' code changes: > > I wasn't sure if the non-RA targets (for which we don't have an effective > target) even have final dump. > If they do as you show, then guess the target guard can go. ACK. Pushed to trunk branch in commit 9451b6c0a941dc44ca6f14ff8565d74fe56cca59 "Enable 'gcc.dg/pr114768.c' for nvptx target [PR114768]", see attached. Grüße Thomas [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: 0001-Enable-gcc.dg-pr114768.c-for-nvptx-target-PR114768.patch --] [-- Type: text/x-diff, Size: 3522 bytes --] From 9451b6c0a941dc44ca6f14ff8565d74fe56cca59 Mon Sep 17 00:00:00 2001 From: Thomas Schwinge <tschwinge@baylibre.com> Date: Fri, 19 Apr 2024 12:32:03 +0200 Subject: [PATCH] Enable 'gcc.dg/pr114768.c' for nvptx target [PR114768] Follow-up to commit 9f295847a9c32081bdd0fe908ffba58e830a24fb "rtlanal: Fix set_noop_p for volatile loads or stores [PR114768]": nvptx does behave in the exactly same way as expected; see 'diff' of before vs. after the 'gcc/rtlanal.cc' code changes: PASS: gcc.dg/pr114768.c (test for excess errors) [-FAIL:-]{+PASS:+} gcc.dg/pr114768.c scan-rtl-dump final "\\(mem/v:" --- 0/pr114768.c.347r.final 2024-04-19 11:34:34.577037596 +0200 +++ ./pr114768.c.347r.final 2024-04-19 12:08:00.118312524 +0200 @@ -13,15 +13,27 @@ ;; entry block defs 1 [%stack] 2 [%frame] 3 [%args] ;; exit block uses 1 [%stack] 2 [%frame] ;; regs ever live -;; ref usage r1={1d,2u} r2={1d,2u} r3={1d,1u} -;; total ref usage 8{3d,5u,0e} in 1{1 regular + 0 call} insns. +;; ref usage r1={1d,3u} r2={1d,3u} r3={1d,2u} r22={1d,1u} r23={1d,2u} +;; total ref usage 16{5d,11u,0e} in 4{4 regular + 0 call} insns. (note 1 0 4 NOTE_INSN_DELETED) (note 4 1 2 2 [bb 2] NOTE_INSN_BASIC_BLOCK) -(note 2 4 3 2 NOTE_INSN_DELETED) +(insn 2 4 3 2 (set (reg/v/f:DI 23 [ p ]) + (unspec:DI [ + (const_int 0 [0]) + ] UNSPEC_ARG_REG)) "source-gcc/gcc/testsuite/gcc.dg/pr114768.c":8:1 14 {load_arg_regdi} + (nil)) (note 3 2 6 2 NOTE_INSN_FUNCTION_BEG) -(note 6 3 10 2 NOTE_INSN_DELETED) -(note 10 6 11 2 NOTE_INSN_EPILOGUE_BEG) -(jump_insn 11 10 12 2 (return) "source-gcc/gcc/testsuite/gcc.dg/pr114768.c":10:1 289 {return} +(insn 6 3 7 2 (set (reg:SI 22 [ _1 ]) + (mem/v:SI (reg/v/f:DI 23 [ p ]) [1 MEM[(volatile int *)p_3(D)]+0 S4 A32])) "source-gcc/gcc/testsuite/gcc.dg/pr114768.c":9:8 6 {*movsi_insn} + (nil)) +(insn 7 6 10 2 (set (mem:SI (reg/v/f:DI 23 [ p ]) [1 *p_3(D)+0 S4 A32]) + (reg:SI 22 [ _1 ])) "source-gcc/gcc/testsuite/gcc.dg/pr114768.c":9:6 6 {*movsi_insn} + (expr_list:REG_DEAD (reg/v/f:DI 23 [ p ]) + (expr_list:REG_DEAD (reg:SI 22 [ _1 ]) + (nil)))) +(note 10 7 13 2 NOTE_INSN_EPILOGUE_BEG) +(note 13 10 11 3 [bb 3] NOTE_INSN_BASIC_BLOCK) +(jump_insn 11 13 12 3 (return) "source-gcc/gcc/testsuite/gcc.dg/pr114768.c":10:1 289 {return} (nil) -> return) (barrier 12 11 0) --- 0/pr114768.s 2024-04-19 11:34:34.577037596 +0200 +++ ./pr114768.s 2024-04-19 12:08:00.118312524 +0200 @@ -13,5 +13,10 @@ { .reg.u64 %ar0; ld.param.u64 %ar0, [%in_ar0]; + .reg.u32 %r22; + .reg.u64 %r23; + mov.u64 %r23, %ar0; + ld.u32 %r22, [%r23]; + st.u32 [%r23], %r22; ret; } PR testsuite/114768 gcc/testsuite/ * gcc.dg/pr114768.c: Enable for nvptx target. --- gcc/testsuite/gcc.dg/pr114768.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gcc/testsuite/gcc.dg/pr114768.c b/gcc/testsuite/gcc.dg/pr114768.c index 2075f0d6b82..ffe3b368638 100644 --- a/gcc/testsuite/gcc.dg/pr114768.c +++ b/gcc/testsuite/gcc.dg/pr114768.c @@ -1,7 +1,7 @@ /* PR rtl-optimization/114768 */ /* { dg-do compile } */ /* { dg-options "-O2 -fdump-rtl-final" } */ -/* { dg-final { scan-rtl-dump "\\\(mem/v:" "final" { target { ! { nvptx*-*-* } } } } } */ +/* { dg-final { scan-rtl-dump "\\\(mem/v:" "final" } } */ void foo (int *p) -- 2.34.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-04-19 10:39 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-04-19 6:24 [PATCH] rtlanal: Fix set_noop_p for volatile loads or stores [PR114768] Jakub Jelinek 2024-04-19 6:33 ` Richard Biener 2024-04-19 10:23 ` Thomas Schwinge 2024-04-19 10:30 ` Jakub Jelinek 2024-04-19 10:39 ` Enable 'gcc.dg/pr114768.c' for nvptx target [PR114768] (was: [PATCH] rtlanal: Fix set_noop_p for volatile loads or stores [PR114768]) Thomas Schwinge
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).