Message ID | YjDbsVPf3IxIzXs7@tucnak |
---|---|
State | New |
Headers | show |
Series | aarch64: Fix up RTL sharing bug in aarch64_load_symref_appropriately [PR104910] | expand |
Jakub Jelinek <jakub@redhat.com> writes: > Hi! > > We unshare all RTL created during expansion, but when > aarch64_load_symref_appropriately is called after expansion like in the > following testcases, we use imm in both HIGH and LO_SUM operands. > If imm is some RTL that shouldn't be shared like a non-sharable CONST, > we get at least with --enable-checking=rtl a checking ICE, otherwise might > just get silently wrong code. > > The following patch fixes that by copying it if it can't be shared. > > Bootstrapped/regtested on aarch64-linux, ok for trunk? > > 2022-03-15 Jakub Jelinek <jakub@redhat.com> > > PR target/104910 > * config/aarch64/aarch64.cc (aarch64_load_symref_appropriately): Copy > imm rtx. > > * gcc.dg/pr104910.c: New test. OK, thanks. I guess the REG_EQUAL notes might need the same treatment, but that's probably a separate patch. Richard --- gcc/config/aarch64/aarch64.cc.jj 2022-02-22 10:38:02.404689359 +0100 > +++ gcc/config/aarch64/aarch64.cc 2022-03-14 12:42:00.218975192 +0100 > @@ -3971,7 +3971,7 @@ aarch64_load_symref_appropriately (rtx d > if (can_create_pseudo_p ()) > tmp_reg = gen_reg_rtx (mode); > > - emit_move_insn (tmp_reg, gen_rtx_HIGH (mode, imm)); > + emit_move_insn (tmp_reg, gen_rtx_HIGH (mode, copy_rtx (imm))); > emit_insn (gen_add_losym (dest, tmp_reg, imm)); > return; > } > --- gcc/testsuite/gcc.dg/pr104910.c.jj 2022-03-14 12:46:20.983327114 +0100 > +++ gcc/testsuite/gcc.dg/pr104910.c 2022-03-14 12:46:06.064535794 +0100 > @@ -0,0 +1,14 @@ > +/* PR target/104910 */ > +/* { dg-do compile } */ > +/* { dg-options "-Os -fno-forward-propagate" } */ > +/* { dg-additional-options "-fstack-protector-all" { target fstack_protector } } */ > + > +void > +bar (void); > + > +void > +foo (int x) > +{ > + if (x) > + bar (); > +} > > Jakub
On Wed, Mar 16, 2022 at 09:10:35AM +0000, Richard Sandiford wrote: > Jakub Jelinek <jakub@redhat.com> writes: > > We unshare all RTL created during expansion, but when > > aarch64_load_symref_appropriately is called after expansion like in the > > following testcases, we use imm in both HIGH and LO_SUM operands. > > If imm is some RTL that shouldn't be shared like a non-sharable CONST, > > we get at least with --enable-checking=rtl a checking ICE, otherwise might > > just get silently wrong code. > > > > The following patch fixes that by copying it if it can't be shared. > > > > Bootstrapped/regtested on aarch64-linux, ok for trunk? > > > > 2022-03-15 Jakub Jelinek <jakub@redhat.com> > > > > PR target/104910 > > * config/aarch64/aarch64.cc (aarch64_load_symref_appropriately): Copy > > imm rtx. > > > > * gcc.dg/pr104910.c: New test. > > OK, thanks. I guess the REG_EQUAL notes might need the same treatment, > but that's probably a separate patch. If it can be invoked after expansion, yes. Though, I think e.g. having the TLS related loads/stores done after expansion is unlikely. Anyway, --enable-checking=rtl ought to catch this. Jakub
--- gcc/config/aarch64/aarch64.cc.jj 2022-02-22 10:38:02.404689359 +0100 +++ gcc/config/aarch64/aarch64.cc 2022-03-14 12:42:00.218975192 +0100 @@ -3971,7 +3971,7 @@ aarch64_load_symref_appropriately (rtx d if (can_create_pseudo_p ()) tmp_reg = gen_reg_rtx (mode); - emit_move_insn (tmp_reg, gen_rtx_HIGH (mode, imm)); + emit_move_insn (tmp_reg, gen_rtx_HIGH (mode, copy_rtx (imm))); emit_insn (gen_add_losym (dest, tmp_reg, imm)); return; } --- gcc/testsuite/gcc.dg/pr104910.c.jj 2022-03-14 12:46:20.983327114 +0100 +++ gcc/testsuite/gcc.dg/pr104910.c 2022-03-14 12:46:06.064535794 +0100 @@ -0,0 +1,14 @@ +/* PR target/104910 */ +/* { dg-do compile } */ +/* { dg-options "-Os -fno-forward-propagate" } */ +/* { dg-additional-options "-fstack-protector-all" { target fstack_protector } } */ + +void +bar (void); + +void +foo (int x) +{ + if (x) + bar (); +}