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

[Jeff Law <law at redhat dot com>]

On 9/3/19 7:13 AM, Richard Biener wrote:
> On Tue, 3 Sep 2019, Richard Biener wrote:
> 
>> On Tue, 3 Sep 2019, Bernd Edlinger wrote:
>>
>>> 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.
>>
>> It should really be so.  IVOPTs created them and asked the backend
>> if it supports it.  But yeah - who knows, I'd have to double check
>> whether IVOPTs is careful here or not - at least I doubt targets
>> w/o movmisalign_optab will never create unaligned TARGET_MEM_REFs...
>>
>>>>         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?
>>
>> Ick.
>>
>>> 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?
>>
>> I'd have to see the result to say...  but I did hope it was
>> going to be a bit simpler.
> 
> I'd say go for the original patch and try the refactoring on top.
Works for me as well.

jeff