cfgrtl: Fix MEM_EXPR update in duplicate_insn_chain [PR114924]

cfgrtl: Fix MEM_EXPR update in duplicate_insn_chain [PR114924]

[Alex Coplan]

[-- Attachment #1: Type: text/plain, Size: 787 bytes --]

Hi,

The PR shows that when cfgrtl.cc:duplicate_insn_chain attempts to
update the MR_DEPENDENCE_CLIQUE information for a MEM_EXPR we can end up
accidentally dropping (e.g.) an ARRAY_REF from the MEM_EXPR and end up
replacing it with the underlying MEM_REF.  This leads to an
inconsistency in the MEM_EXPR information, and could lead to wrong code.

While the walk down to the MEM_REF is necessary to update
MR_DEPENDENCE_CLIQUE, we should use the outer tree expression for the
MEM_EXPR.  This patch does that.

Bootstrapped/regtested on aarch64-linux-gnu, no regressions.  OK for
trunk?  What about backports?

Thanks,
Alex

gcc/ChangeLog:

	PR rtl-optimization/114924
	* cfgrtl.cc (duplicate_insn_chain): When updating MEM_EXPRs,
	don't strip (e.g.) ARRAY_REFs from the final MEM_EXPR.

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 740 bytes --]

diff --git a/gcc/cfgrtl.cc b/gcc/cfgrtl.cc
index 304c429c99b..a5dc3512159 100644
--- a/gcc/cfgrtl.cc
+++ b/gcc/cfgrtl.cc
@@ -4432,12 +4432,13 @@ duplicate_insn_chain (rtx_insn *from, rtx_insn *to,
 			   since MEM_EXPR is shared so make a copy and
 			   walk to the subtree again.  */
 			tree new_expr = unshare_expr (MEM_EXPR (*iter));
+			tree orig_new_expr = new_expr;
 			if (TREE_CODE (new_expr) == WITH_SIZE_EXPR)
 			  new_expr = TREE_OPERAND (new_expr, 0);
 			while (handled_component_p (new_expr))
 			  new_expr = TREE_OPERAND (new_expr, 0);
 			MR_DEPENDENCE_CLIQUE (new_expr) = newc;
-			set_mem_expr (const_cast <rtx> (*iter), new_expr);
+			set_mem_expr (const_cast <rtx> (*iter), orig_new_expr);
 		      }
 		  }
 	    }

Re: cfgrtl: Fix MEM_EXPR update in duplicate_insn_chain [PR114924]

[Richard Biener]

On Thu, 2 May 2024, Alex Coplan wrote:

> Hi,
> 
> The PR shows that when cfgrtl.cc:duplicate_insn_chain attempts to
> update the MR_DEPENDENCE_CLIQUE information for a MEM_EXPR we can end up
> accidentally dropping (e.g.) an ARRAY_REF from the MEM_EXPR and end up
> replacing it with the underlying MEM_REF.  This leads to an
> inconsistency in the MEM_EXPR information, and could lead to wrong code.
> 
> While the walk down to the MEM_REF is necessary to update
> MR_DEPENDENCE_CLIQUE, we should use the outer tree expression for the
> MEM_EXPR.  This patch does that.
> 
> Bootstrapped/regtested on aarch64-linux-gnu, no regressions.  OK for
> trunk?  What about backports?

OK for trunk and branches, including 14, I think this is quite safe.

Thanks,
Richard.

> Thanks,
> Alex
> 
> gcc/ChangeLog:
> 
> 	PR rtl-optimization/114924
> 	* cfgrtl.cc (duplicate_insn_chain): When updating MEM_EXPRs,
> 	don't strip (e.g.) ARRAY_REFs from the final MEM_EXPR.
> 

-- 
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)