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.edlin...@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.edlin...@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.edlin...@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
