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.
Thus, OK.
Thanks,
Richard.
