Re: [PATCH] Fix some more alignment bugs in the midde-end (PR 91603, 91612, 91613)

[Richard Biener <rguenther at suse dot de>]

On Tue, 3 Sep 2019, Bernd Edlinger wrote:

> On 9/3/19 9:05 AM, Jakub Jelinek wrote:
> > On Tue, Sep 03, 2019 at 07:02:53AM +0000, Bernd Edlinger wrote:
> >> 2019-09-03  Bernd Edlinger  <bernd.edlinger@hotmail.de>
> >>
> >> 	PR middle-end/91603
> >> 	PR middle-end/91612
> >> 	PR middle-end/91613
> >> 	* expr.c (expand_expr_real_1): decl_p_1): Refactor into...
> >> 	(non_mem_decl_p): ...this.
> >> 	(mem_ref_refers_to_non_mem_p): Handle DECL_P as well ase MEM_REF.
> >> 	(expand_assignment): Call mem_ref_referes_to_non_mem_p
> >> 	unconditionally as before.
> > 
> > Not a review, just questioning the ChangeLog entry.
> > What is the "decl_p_1): " in there?  Also, the ChangeLog mentions many
> > functions, but the patch in reality just modifies expand_expr_real_1
> > and nothing else.
> > 
> 
> Ah, sorry, this is of course wrong, (I forgot to complete the sentence,
> and later forgot to check it again)....
> 
> 
> This is what I actually wanted to say:
> 
> 2019-09-03  Bernd Edlinger  <bernd.edlinger@hotmail.de>
> 
>         PR middle-end/91603
>         PR middle-end/91612
>         PR middle-end/91613
>         * expr.c (expand_expr_real_1): Handle unaligned decl_rtl
>         and SSA_NAME referring to CONSTANT_P correctly.
> 
> testsuite:
> 2019-09-03  Bernd Edlinger  <bernd.edlinger@hotmail.de>
> 
>         PR middle-end/91603
>         * testsuite/gcc.target/arm/pr91603.c: New test.

@@ -10062,7 +10062,43 @@ expand_expr_real_1 (tree exp, rtx target, 
machine_
        {
          if (exp && MEM_P (temp) && REG_P (XEXP (temp, 0)))
            mark_reg_pointer (XEXP (temp, 0), DECL_ALIGN (exp));
+       }
+      else if (MEM_P (decl_rtl))
+       temp = decl_rtl;
 
+      if (temp != 0)
+       {
+         if (MEM_P (temp)
+             && modifier != EXPAND_WRITE
+             && modifier != EXPAND_MEMORY
+             && modifier != EXPAND_INITIALIZER
+             && modifier != EXPAND_CONST_ADDRESS
+             && modifier != EXPAND_SUM
+             && !inner_reference_p
+             && mode != BLKmode
+             && MEM_ALIGN (temp) < GET_MODE_ALIGNMENT (mode))

So other places ([TARGET_]MEM_REF cases) use "just"

        if (modifier != EXPAND_WRITE
            && modifier != EXPAND_MEMORY
            && !inner_reference_p
            && mode != BLKmode
            && align < GET_MODE_ALIGNMENT (mode))

I also wonder if you can split out all this common code to
a function (the actual unaligned expansion, that is) and call
it from those places (where the TARGET_MEM_REF case misses the
slow_unaligned_access case - presumably wanting to "assert"
that this doesn't happen.

            /* If the target does not have special handling for unaligned
               loads of mode then it can use regular moves for them.  */

Richard.

+           {
+             enum insn_code icode;
+
+             if ((icode = optab_handler (movmisalign_optab, mode))
+                 != CODE_FOR_nothing)
+               {
+                 class expand_operand ops[2];
+
+                 /* We've already validated the memory, and we're 
creating
a
+                    new pseudo destination.  The predicates really can't
fail,
+                    nor can the generator.  */
+                 create_output_operand (&ops[0], NULL_RTX, mode);
+                 create_fixed_operand (&ops[1], temp);
+                 expand_insn (icode, 2, ops);
+                 temp = ops[0].value;
+               }
+             else if (targetm.slow_unaligned_access (mode, MEM_ALIGN
(temp)))
+               temp = extract_bit_field (temp, GET_MODE_BITSIZE (mode),
+                                         0, unsignedp, NULL_RTX,
+                                         mode, mode, false, NULL);
+           }
+
          return temp;
        }