[PATCHv2] fwprop: Avoid volatile defines to be propagated

[PATCHv2] fwprop: Avoid volatile defines to be propagated

[HAO CHEN GUI]

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?

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;
+}

Re: [PATCHv2] fwprop: Avoid volatile defines to be propagated

[Richard Sandiford]

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;
> +}