This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH][GCC][AARCH64]Bad code-gen for structure/block/unaligned memory access


On 07/03/2017 12:17 AM, Tamar Christina wrote:
> Hi All,
> 
> Sorry I just realized I never actually sent the updated patch...
AFAICT this never got resolved.

I'll let the aarch64 folks own those changes.  I'm going to focus just
on the small change to expr.c where you change the computation of
bitsize in copy_blkmode_to_reg.

My biggest worry is that this could end up breaking STRICT_ALIGNMENT
targets.  Thankfully, the default definition of SLOW_UNALIGNED_ACCESS is
STRICT_ALIGNMENT.  So on a STRICT_ALIGNMENT target that does not define
SLOW_UNALIGNED_ACCESS we should OK.  I also did a quick scan of
STRICT_ALIGNMENT targets that override SLOW_UNALIGNED_ACCESS and they
seem reasonable.

So I'll ack the expr.c change.  You'll need to chase down an ack on the
aarch64 specific changes.


jeff

> 
> So here it is :)
> 
> Regards,
> Tamar
> ________________________________________
> From: gcc-patches-owner@gcc.gnu.org <gcc-patches-owner@gcc.gnu.org> on behalf of Tamar Christina <Tamar.Christina@arm.com>
> Sent: Friday, June 9, 2017 4:51:52 PM
> To: Richard Sandiford
> Cc: GCC Patches; nd; James Greenhalgh; Richard Earnshaw; Marcus Shawcroft
> Subject: RE: [PATCH][GCC][AARCH64]Bad code-gen for structure/block/unaligned memory access
> 
>>> +  /* Optimize routines for MEM to REG copies.  */  if (n < 8 &&
>>> + !REG_P (XEXP (operands[0], 0)))
>>
>> This seems to be checking that the address of the original destination
>> memory isn't a plain base register.  Why's it important to reject that case but
>> allow e.g. base+offset?
> 
> this function is (as far as I could tell) only being called with two types of destinations:
> a location on the stack or a plain register.
> 
> When the destination is a register such as with
> 
> void Fun3(struct struct3 foo3)
> {
>   L3 = foo3;
> }
> 
> You run into the issue you had pointed to below where we might write too much. Ideally the constraint
> I would like is to check if the destination is either a new register (no data) or that the structure was padded.
> 
> I couldn't figure out how to do this and the gains over the existing code for this case was quite small.
> So I just disallowed it leaving it to the existing code, which isn't so bad, only 1 extra instruction.
> 
>>
>>> +   {
>>> +     unsigned int max_align = UINTVAL (operands[2]);
>>> +     max_align = n < max_align ? max_align : n;
>>
>> Might be misunderstanding, but isn't max_align always equal to n here, since
>> n was set by:
> 
> Correct, previously this patch was made to allow n < 16. These were left over from the cleanup.
> I'll correct.
> 
>>
>>> +     result = gen_rtx_IOR (dest_mode, reg, result);
>>> +      }
>>> +
>>> +    dst = adjust_address (dst, dest_mode, 0);
>>> +    emit_insn (gen_move_insn (dst, result));
>>
>> dest_mode was chosen by smallest_mode_for_size, so can be bigger than n.
>> Doesn't that mean that we'll write beyond the end of the copy region when
>> n is an awkward number?
> 
> Yes, see my answer above. For the other case, when we write onto a location on the stack, this is fine
> due to the alignment.
> 
>>
>>> diff --git a/gcc/expr.c b/gcc/expr.c
>>> index
>>>
>> 91d7ea217229fac62380b5d4b646961bf7c836c1..b1df4651e7942346007cda1cce
>> 8e
>>> e5a19297ab16 100644
>>> --- a/gcc/expr.c
>>> +++ b/gcc/expr.c
>>> @@ -2743,7 +2743,9 @@ copy_blkmode_to_reg (machine_mode mode,
>> tree
>>> src)
>>>
>>>    n_regs = (bytes + UNITS_PER_WORD - 1) / UNITS_PER_WORD;
>>>    dst_words = XALLOCAVEC (rtx, n_regs);
>>> -  bitsize = MIN (TYPE_ALIGN (TREE_TYPE (src)), BITS_PER_WORD);
>>> +  bitsize = BITS_PER_WORD;
>>> +  if (SLOW_UNALIGNED_ACCESS (BLKmode, TYPE_ALIGN (TREE_TYPE
>> (src))))
>>> +    bitsize = MIN (TYPE_ALIGN (TREE_TYPE (src)), BITS_PER_WORD);
>>
>> I think this ought to be testing word_mode instead of BLKmode.
>> (Testing BLKmode doesn't really make sense in general, because the mode
>> doesn't have a meaningful alignment.)
> 
> Ah, yes that makes sense. I'll update the patch.
> 
> New patch is validating and will submit it soon.
> 
>>
>> Thanks,
>> Richard


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]