diff mbox

Fix RTL DSE (PR rtl-optimization/68955)

Message ID 20160118093239.GT3017@tucnak.redhat.com
State New
Headers show

Commit Message

Jakub Jelinek Jan. 18, 2016, 9:32 a.m. UTC 
On Mon, Jan 18, 2016 at 10:06:44AM +0100, Eric Botcazou wrote:
> > The following testcase is miscompiled on i686-linux at -O3.
> > The bug is in DSE record_store, which for group_id < 0 uses mem_addr
> > set to result of get_addr (base->val_rtx) (plus optional offset),
> > which is fine for canon_true_dependence with other MEMs in that function,
> > but we also store that address in store_info.  The problem is if later on
> > e.g. some read uses the same e.g. hard register as get_addr returned, but
> > that register contains at that later point a different value.
> > canon_true_dependence then happily returns the read does not alias the
> > store, although it might.
> > The fix is to store the VALUE (plus optional offset) into
> > store_info->mem_addr instead, then at some later insn when get_addr is
> > called on it it will either return the same register or expression (if it
> > has not changed), or some different one otherwise.
> 
> I presume that the origin of the bug is:
> 
>       /* get_addr can only handle VALUE but cannot handle expr like:
> 	 VALUE + OFFSET, so call get_addr to get original addr for
> 	 mem_addr before plus_constant.  */
>       mem_addr = get_addr (mem_addr);
>       if (offset)
> 	mem_addr = plus_constant (get_address_mode (mem), mem_addr, offset);
> 
> both in record_store and check_mem_read_rtx, so I wonder if we shouldn't bite 
> the bullet and try enhancing get_addr since it's a mainline-only regression.

So, do you suggest to tweak get_addr like the patch below, and remove the
  mem_addr = get_addr (mem_addr);
line above and the comment?



	Jakub

Comments

Eric Botcazou Jan. 18, 2016, 10:40 a.m. UTC | #1 
> So, do you suggest to tweak get_addr like the patch below, and remove the
>   mem_addr = get_addr (mem_addr);
> line above and the comment?

Yes, exactly.  And if that doesn't easily work, then go for your solution and 
add a blurb to the comment explaining why get_addr cannot be easily changed.
diff mbox

Patch

--- gcc/alias.c.jj	2016-01-14 17:01:09.316932111 +0100
+++ gcc/alias.c	2016-01-18 10:30:46.780994699 +0100
@@ -2203,7 +2203,23 @@  get_addr (rtx x)
   struct elt_loc_list *l;
 
   if (GET_CODE (x) != VALUE)
-    return x;
+    {
+      if ((GET_CODE (x) == PLUS || GET_CODE (x) == MINUS)
+	  && GET_CODE (XEXP (x, 0)) == VALUE
+	  && CONST_SCALAR_INT_P (XEXP (x, 1)))
+	{
+	  rtx op0 = get_addr (XEXP (x, 0));
+	  if (op0 != XEXP (x, 0))
+	    {
+	      if (GET_CODE (x) == PLUS
+		  && GET_CODE (XEXP (x, 1)) == CONST_INT)
+		return plus_constant (GET_MODE (x), op0, INTVAL (XEXP (x, 1)));
+	      return simplify_gen_binary (GET_CODE (x), GET_MODE (x),
+					  op0, XEXP (x, 1));
+	    }
+	}
+      return x;
+    }
   v = CSELIB_VAL_PTR (x);
   if (v)
     {