diff mbox series

RFA: avoid changing MEMs to mode with higher alignment than underlying memory

Message ID 600F7774.3060700@riscy-ip.com
State New
Headers show
Series RFA: avoid changing MEMs to mode with higher alignment than underlying memory | expand

Commit Message

Joern Wolfgang Rennecke Jan. 26, 2021, 1:59 a.m. UTC 
esirisc has a sub-target that has DImode with 4 byte alignment, and
DFmode with 8 byte alignment.  For code like g++.dg/torture/pr39713.C ,
the optimizes changes the mode of a MEM from DImode to DFmode even if
the required alignment is not available.

Appended is is the target-independent part of the fix.  Required target
changes were to make sure that (subreg:DF: (mem:DI ...)) is handled or
rejected as an operandad as appropriate, and a secondary reload to a
suitable register and in a suitable mode are done where needed
(GENERAL_REGS in DImode in our case).

Bootstrapped and regression tested in gcc version
d6f1cf644c45b76a27b6a6869dedaa030e3c7570 on x86_64 GNU/Linux.
2021-01-14  Joern Rennecke  <joern.rennecke@riscy-ip.com>

        * combine.c (gen_lowpart_for_combine): Reject change to MEM with mode
        with higher alignment than given.
        * expr.c (expand_expr_real_1 <VIEW_CONVERT_EXPR>):
        When don't discard mode of a MEM before checking alignment.
        For STRICT_ALIGNMENT, try to load a reg in the inner mode.
        * simplify-rtx.c (simplify_subreg): Reject change to MEM with mode with
        higher alignment than given.

Comments

Richard Biener Jan. 26, 2021, 7:56 a.m. UTC | #1 
On Tue, Jan 26, 2021 at 3:07 AM Joern Wolfgang Rennecke
<joern.rennecke@riscy-ip.com> wrote:
>
> esirisc has a sub-target that has DImode with 4 byte alignment, and
> DFmode with 8 byte alignment.  For code like g++.dg/torture/pr39713.C ,
> the optimizes changes the mode of a MEM from DImode to DFmode even if
> the required alignment is not available.
>
> Appended is is the target-independent part of the fix.  Required target
> changes were to make sure that (subreg:DF: (mem:DI ...)) is handled or
> rejected as an operandad as appropriate, and a secondary reload to a
> suitable register and in a suitable mode are done where needed
> (GENERAL_REGS in DImode in our case).

Hmm, IIRC the mode of a mem is only relevant if there's no MEM_ATTRs
(because then the default attrs of the mode apply), targets are responsible
to reject MEMs they do not implement because of alignment constraints.

So I wonder whether the

+      if (GET_MODE_ALIGNMENT (omode) > GET_MODE_ALIGNMENT (imode)
+         && GET_MODE_ALIGNMENT (omode) > MEM_ALIGN (x))
+       goto fail;

changes do not paper over either a target failure to reject built mems or
the optimizers not properly recognizing the result and thus verifying
validity of the generated MEM?  At least the above gen_lowpart_for_combine
will also inhibit the transform on DImode -> DFmode on i?86 with -malign-double?
I realize the condition is unusual (and thus unlikely to trigger), but still.

Richard.

> Bootstrapped and regression tested in gcc version
> d6f1cf644c45b76a27b6a6869dedaa030e3c7570 on x86_64 GNU/Linux.
diff mbox series

Patch

diff --git a/gcc/combine.c b/gcc/combine.c
index 3294575..966fa4a 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -11793,6 +11793,10 @@  gen_lowpart_for_combine (machine_mode omode, rtx x)
 	  || mode_dependent_address_p (XEXP (x, 0), MEM_ADDR_SPACE (x)))
 	goto fail;
 
+      if (GET_MODE_ALIGNMENT (omode) > GET_MODE_ALIGNMENT (imode)
+	  && GET_MODE_ALIGNMENT (omode) > MEM_ALIGN (x))
+	goto fail;
+
       /* If we want to refer to something bigger than the original memref,
 	 generate a paradoxical subreg instead.  That will force a reload
 	 of the original memref X.  */
diff --git a/gcc/expr.c b/gcc/expr.c
index 04ef5ad..1f0117c 100644
--- a/gcc/expr.c
+++ b/gcc/expr.c
@@ -11362,7 +11362,9 @@  expand_expr_real_1 (tree exp, rtx target, machine_mode tmode,
 	    {
 	      if (!REG_P (op0) && !MEM_P (op0))
 		op0 = force_reg (GET_MODE (op0), op0);
-	      op0 = gen_lowpart (mode, op0);
+	      rtx tmp = gen_lowpart (mode, op0);
+	      if (!MEM_P (tmp))
+		op0 = tmp;
 	    }
 	}
       /* If both types are integral, convert from one mode to the other.  */
@@ -11383,6 +11385,8 @@  expand_expr_real_1 (tree exp, rtx target, machine_mode tmode,
 	     force_const_mem for constants because we don't allow pool
 	     constants to change mode.  */
 	  tree inner_type = TREE_TYPE (treeop0);
+	  /* ??? Should probably modify the INNER_TYPE to bigger alignemnt if
+	     TYPE requires it.  */
 
 	  gcc_assert (!TREE_ADDRESSABLE (exp));
 
@@ -11430,6 +11434,14 @@  expand_expr_real_1 (tree exp, rtx target, machine_mode tmode,
 		}
 	      else if (STRICT_ALIGNMENT)
 		{
+		  machine_mode inner_mode = TYPE_MODE (TREE_TYPE (treeop0));
+		  if (GET_MODE (op0) == inner_mode
+		      && targetm.can_change_mode_class
+			   (inner_mode, mode, GENERAL_REGS))
+		    {
+		      op0 = force_reg (inner_mode, op0);
+		      return gen_lowpart (mode, op0);
+		    }
 		  poly_uint64 mode_size = GET_MODE_SIZE (mode);
 		  poly_uint64 temp_size = mode_size;
 		  if (GET_MODE (op0) != BLKmode)
diff --git a/gcc/simplify-rtx.c b/gcc/simplify-rtx.c
index 571e233..766e0d0 100644
--- a/gcc/simplify-rtx.c
+++ b/gcc/simplify-rtx.c
@@ -7206,6 +7206,9 @@  simplify_context::simplify_subreg (machine_mode outermode, rtx op,
 
   if (MEM_P (op)
       && ! mode_dependent_address_p (XEXP (op, 0), MEM_ADDR_SPACE (op))
+      /* Don't imply a larger alignment than is available.  */
+      && (GET_MODE_ALIGNMENT (outermode) <= GET_MODE_ALIGNMENT (innermode)
+	  || GET_MODE_ALIGNMENT (outermode) <= MEM_ALIGN (op))
       /* Allow splitting of volatile memory references in case we don't
          have instruction to move the whole thing.  */
       && (! MEM_VOLATILE_P (op)