| Message ID | c314b5ca-8230-441c-bfd8-ffa5ea20a8e4@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PATCHv2] fwprop: Avoid volatile defines to be propagated | expand |
HAO CHEN GUI <guihaoc@linux.ibm.com> writes: > Hi, > This patch tries to fix a potential problem which is raised by the patch > for PR111267. The volatile asm operand tries to be propagated to a single > set insn with the patch for PR111267. The volatile asm operand might be > executed for multiple times if the define insn isn't eliminated after > propagation. Now set_src_cost comparison might reject such propagation. > But it has the chance to be taken after replacing set_src_cost with insn > cost. Actually I found the problem in testing my patch which replacing > set_src_cost with insn_cost in fwprop pass. > > Compared to the last version, the check volatile_insn_p is replaced with > volatile_refs_p in order to check volatile memory reference also. > https://gcc.gnu.org/pipermail/gcc-patches/2024-February/646482.html > > Bootstrapped and tested on x86 and powerpc64-linux BE and LE with no > regressions. Is it OK for the trunk? OK, thanks. I'm not sure this fixes a known regression, but IMO the barrier should be lower for things that fix loss of volatility, since it's usually so hard to observe the effect in a determinstic way. Richard > > Thanks > Gui Haochen > > ChangeLog > fwprop: Avoid volatile defines to be propagated > > The patch for PR111267 (commit id 86de9b66480b710202a2898cf513db105d8c432f) > which introduces an exception for propagation on single set insn. The > propagation which might not be profitable (checked by profitable_p) is still > allowed to be propagated to single set insn. It has a potential problem > that a volatile operand might be propagated to a single set insn. If the > define insn is not eliminated after propagation, the volatile operand will > be executed for multiple times. This patch fixes the problem by skipping > volatile set source rtx in propagation. > > gcc/ > * fwprop.cc (forward_propagate_into): Return false for volatile set > source rtx. > > gcc/testsuite/ > * gcc.target/powerpc/fwprop-1.c: New. > > patch.diff > diff --git a/gcc/fwprop.cc b/gcc/fwprop.cc > index 7872609b336..cb6fd6700ca 100644 > --- a/gcc/fwprop.cc > +++ b/gcc/fwprop.cc > @@ -854,6 +854,8 @@ forward_propagate_into (use_info *use, bool reg_prop_only = false) > > rtx dest = SET_DEST (def_set); > rtx src = SET_SRC (def_set); > + if (volatile_refs_p (src)) > + return false; > > /* Allow propagations into a loop only for reg-to-reg copies, since > replacing one register by another shouldn't increase the cost. > diff --git a/gcc/testsuite/gcc.target/powerpc/fwprop-1.c b/gcc/testsuite/gcc.target/powerpc/fwprop-1.c > new file mode 100644 > index 00000000000..07b207f980c > --- /dev/null > +++ b/gcc/testsuite/gcc.target/powerpc/fwprop-1.c > @@ -0,0 +1,15 @@ > +/* { dg-do compile } */ > +/* { dg-options "-O1 -fdump-rtl-fwprop1-details" } */ > +/* { dg-final { scan-rtl-dump-not "propagating insn" "fwprop1" } } */ > + > +/* Verify that volatile asm operands doesn't be propagated. */ > +long long foo () > +{ > + long long res; > + __asm__ __volatile__( > + "" > + : "=r" (res) > + : > + : "memory"); > + return res; > +}
diff --git a/gcc/fwprop.cc b/gcc/fwprop.cc index 7872609b336..cb6fd6700ca 100644 --- a/gcc/fwprop.cc +++ b/gcc/fwprop.cc @@ -854,6 +854,8 @@ forward_propagate_into (use_info *use, bool reg_prop_only = false) rtx dest = SET_DEST (def_set); rtx src = SET_SRC (def_set); + if (volatile_refs_p (src)) + return false; /* Allow propagations into a loop only for reg-to-reg copies, since replacing one register by another shouldn't increase the cost. diff --git a/gcc/testsuite/gcc.target/powerpc/fwprop-1.c b/gcc/testsuite/gcc.target/powerpc/fwprop-1.c new file mode 100644 index 00000000000..07b207f980c --- /dev/null +++ b/gcc/testsuite/gcc.target/powerpc/fwprop-1.c @@ -0,0 +1,15 @@ +/* { dg-do compile } */ +/* { dg-options "-O1 -fdump-rtl-fwprop1-details" } */ +/* { dg-final { scan-rtl-dump-not "propagating insn" "fwprop1" } } */ + +/* Verify that volatile asm operands doesn't be propagated. */ +long long foo () +{ + long long res; + __asm__ __volatile__( + "" + : "=r" (res) + : + : "memory"); + return res; +}