On Tue, 3 Apr 2018, Alan Modra wrote: > On Tue, Apr 03, 2018 at 01:01:23PM +0200, Richard Biener wrote: > > On Fri, 30 Mar 2018, Peter Bergner wrote: > > > > > On 3/29/18 9:35 AM, Alan Modra wrote: > > > > On Thu, Nov 16, 2017 at 03:27:01PM +0000, Tamar Christina wrote: > > > >> --- a/gcc/expr.c > > > >> +++ b/gcc/expr.c > > > >> @@ -2769,7 +2769,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 (targetm.slow_unaligned_access (word_mode, TYPE_ALIGN (TREE_TYPE > > > >> (src)))) > > > >> + bitsize = MIN (TYPE_ALIGN (TREE_TYPE (src)), BITS_PER_WORD); > > > >> > > > >> /* Copy the structure BITSIZE bits at a time. */ > > > >> for (bitpos = 0, xbitpos = padding_correction; > > > > > > > > I believe this patch is wrong. Please revert. See the PR84762 > > > > testcase. > > > > > > > > There are two problems. Firstly, if padding_correction is non-zero, > > > > then xbitpos % BITS_PER_WORD is non-zero and in > > > > > > > > store_bit_field (dst_word, bitsize, xbitpos % BITS_PER_WORD, > > > > 0, 0, word_mode, > > > > extract_bit_field (src_word, bitsize, > > > > bitpos % BITS_PER_WORD, 1, > > > > NULL_RTX, word_mode, > > > > word_mode, > > > > false, NULL), > > > > false); > > > > > > > > the stored bit-field exceeds the destination register size. You could > > > > fix that by making bitsize the gcd of bitsize and padding_correction. > > > > > > > > However, that doesn't fix the second problem which is that the > > > > extracted bit-field can exceed the source size. That will result in > > > > rubbish being read into a register. > > > > > > FYI, I received an automated response saying Tamar is away on vacation > > > with no return date specified. That means he won't be able to revert > > > the patch. What do we do now? > > > > The code before the change already looks fishy to me. > > Yes, it is fishy so far as the code in the loop relies on alignment > determining a small enough bitsize. Adding > > if (bytes % UNITS_PER_WORD != 0) > bitsize = gcd (bitsize, (bytes % UNITS_PER_WORD) * BITS_PER_UNIT); > > after bitsize is calculated from alignment would make the code > correct, I believe. But I think that will fail the testcase Tamar > added. > > > x = expand_normal (src); > > > > so what do we expect this to expand to in general? Fortunately > > it seems there are exactly two callers so hopefully a > > gcc_assert (MEM_P (x)) would work? > > > > The fishy part is looking at TYPE_ALIGN (TREE_TYPE (src)). > > > > In case x is a MEM we should use MEM_ALIGN and if not then we > > shouldn't do anything but just use word_mode here. > > No! You can't use a bitsize of BITS_PER_WORD here. See the code I > quoted above.
Ah, so it should possibly use TYPE_SIZE instead of TYPE_ALIGN, otherwise it will handle the over-aligned case (align to 8 bytes, size 4 bytes) incorrectly as well? Richard.
