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

[Bernd Edlinger <bernd dot edlinger at hotmail dot de>]

On 9/3/19 1:12 PM, Richard Biener wrote:
> 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"
> 

Yes, interesting all of them do slightly different things.
I started with cloning the MEM_REF case, but it ran immediately
into issues with this assert here:

          result = expand_expr (exp, target, tmode,
                                modifier == EXPAND_INITIALIZER
                                ? EXPAND_INITIALIZER : EXPAND_CONST_ADDRESS);

          /* If the DECL isn't in memory, then the DECL wasn't properly
             marked TREE_ADDRESSABLE, which will be either a front-end
             or a tree optimizer bug.  */

          gcc_assert (MEM_P (result));
          result = XEXP (result, 0);

which implies that I need to add EXPAND_INITIALIZER and EXPAND_CONST_ADDRESS,
but since the code immediately above also has an exception of EXPAND_SUM:

      else if (MEM_P (decl_rtl) && modifier != EXPAND_INITIALIZER)
        {
          if (alt_rtl)
            *alt_rtl = decl_rtl;
          decl_rtl = use_anchored_address (decl_rtl);
          if (modifier != EXPAND_CONST_ADDRESS
              && modifier != EXPAND_SUM

I thought it I need to add also an exception for EXPAND_SUM.

Probably there is a reason why TARGET_MEM_REF does not need the
extract_bit_field stuff, when I read the comment here:

            /* If the target does not have special handling for unaligned
               loads of mode then it can use regular moves for them.  */
            && ((icode = optab_handler (movmisalign_optab, mode))
                != CODE_FOR_nothing))

it is just, I don't really believe it.

>         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.  */
> 

Actually there is still a small difference to the MEM_REF expansion,
see the alt_rtl and the EXPAND_STACK_PARAM:

              temp = extract_bit_field (temp, GET_MODE_BITSIZE (mode),
                                        0, TYPE_UNSIGNED (TREE_TYPE (exp)),
                                        (modifier == EXPAND_STACK_PARM
                                         ? NULL_RTX : target),
                                        mode, mode, false, alt_rtl);


TARGET_MEM_REF does not do extract_bit_field at all,
while I think ignoring target and alt_rtl in the DECL_P case is safe,
target, because it is at most a missed optimization, and
alt_rtl because it should already be handled above?
But if I pass target I cannot simply ignore alt_rtl any more?

Well, I could pass target and alt_rtl differently each time.

should I still try to factor that into a single function, it will have
around 7 parameters?


Bernd.