[PATCH] PR target/96759 - Handle global variable assignment from misaligned structure/PARALLEL return values.

[Richard Biener]

On Thu, 10 Sep 2020, Kito Cheng wrote:

> Hi Richard:
> 
> Thanks for your review :)
> 
> > riscv doesn't seem to have movmisalign and I don't see why movmisalign
> > should not support a TImode parallel RHS so at least you should
> > put this check after the icode != CODE_FOR_nothing check?
> 
> RISC-V has an option `-mno-strict-align` to enable mis-aligned access,
> but we didn't provide movmisalign pattern, I guess might be another issue,
> I tried to add movmisalign pattern and then it will feed PARALLEL into that,
> and got into a similar situation again, most targets are not except
> got PARALLEL,
> so that's the reason why I put before icode != CODE_FOR_nothing check.
> 
> > Also wouldn't it be better to support PARALLEL from within
> > store_bit_field?
> For the above reason, I think store_bit_field supports PARALLEL is  not enough.
> 
> > After all this is a misaligned access and
> > since you didn't quote the RTL involved I'm guessing that
> > emit_group_store knows nothing about this fact.
> 
> Oh, good point, how do you think about loading into temp and then
> storing it in the DECL, so that we could use original logic to
> handle misaligned store.
> 
> A PoC for this idea, didn't fully tested yet:
> 
> diff --git a/gcc/expr.c b/gcc/expr.c
> index 1a15f24b3979..6ef97740c764 100644
> --- a/gcc/expr.c
> +++ b/gcc/expr.c
> @@ -5166,8 +5166,19 @@ expand_assignment (tree to, tree from, bool nontemporal)
>          || targetm.slow_unaligned_access (mode, align)))
>     {
>       rtx reg, mem;
> +      push_temp_slots ();
> 
>       reg = expand_expr (from, NULL_RTX, VOIDmode, EXPAND_NORMAL);

I'd say there must be (wishful thinking) some expand_expr
modifier that guarantees this?  Alternatively use

     reg = force_reg (mode, reg);

instead of assign_stack_temp & emit_group_store?

That said, I don't know very much about the fancy ways to handle
stores from PARALLEL - but doesn't PARALLEL mean we can just take
any of its members as source?  How does 'reg' look like exactly
in your case?  What does 'to' expand to?

Richard.

> +
> +      if (GET_CODE (reg) == PARALLEL)
> +       {
> +         rtx temp = assign_stack_temp (mode,
> +                                       GET_MODE_SIZE (mode));
> +         emit_group_store (temp, reg, TREE_TYPE (from),
> +                          int_size_in_bytes (TREE_TYPE (from)));
> +         reg = temp;
> +       }
> +
>       reg = force_not_mem (reg);
>       mem = expand_expr (to, NULL_RTX, VOIDmode, EXPAND_WRITE);
>       if (TREE_CODE (to) == MEM_REF && REF_REVERSE_STORAGE_ORDER (to))
> @@ -5186,6 +5197,7 @@ expand_assignment (tree to, tree from, bool nontemporal)
>       else
>        store_bit_field (mem, GET_MODE_BITSIZE (mode), 0, 0, 0, mode, reg,
>                         false);
> +      pop_temp_slots ();
>       return;
>     }
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)