Re: [PATCH/expand] PR64011 Adjust bitsize when partial overflow happen for big-endian

[Jiong Wang <jiong dot wang at arm dot com>]

On 09/01/15 05:46, Jeff Law wrote:
> On 12/30/14 03:21, Jiong Wang wrote:
> > PR64011 is actually a general problem on all target support bit insertion
> > instructions.
> >
> > we overflow check at the start of store_bit_field_1, but that only check
> > the
> > situation where the field lies completely outside the register, while
> > there do
> > have situation where the field lies partly in the register, we need to
> > adjust
> > bitsize for this partial overflow situation. Without this fix,
> > pr48335-2.c on
> > big-endian will broken on those arch support bit insert instruction,
> > like arm, aarch64.
> >
> > the testcase is just pr48335-2.c, before this patch is will ICE on arm
> > and =
> > generate
> > invalid assembly on AArch64. after this patch, problem gone away.
> >
> > ok for trunk?
> >
> > bootstrap OK on x86-64 && aarch64.
> > no regression on x86-64
> >
> > thanks.
> >
> > gcc/
> >      PR64011
> >      * expmed.c (store_bit_field_using_insv): Adjust bitsize when there
> > is partial overflow.
> Why adjust here the size of the stored field?  Doesn't this end up
> storing less information?
>
> If those bits are still within the object, even if the object is by some
> means living in a mixture of registers and memory, then don't we need to
> set them all?
>
> If those bits are outside the object, then isn't the source simply
> broken because it's writing data outside the bounds of the given object?
>
> Am I  missing something here?

Jeff,

  thanks for review and the questions.

the bug testcase is
===================

typedef short U __attribute__((may_alias, aligned (1)));
                                                                                                                      
struct S
{
  _Complex float d __attribute__((aligned (8)));
};
                                                                                                                      
void bar(struct S);
                                                                                                                      
void f5 (int x)
{
  struct S s;
  ((U *)((char *) &s.d + 1))[3] = x;
  bar (s);
}

and the final tree is:
======================
;; Function f5 (f5, funcdef_no=0, decl_uid=2608, cgraph_uid=0, symbol_order=0)
                                                                                                                      
f5 (int x)
{
  struct S s;
  short int _2;
                                                                                                                      
  <bb 2>:
  _2 = (short int) x_1(D);
  MEM[(U * {ref-all})&s + 7B] = _2; <-- A
  bar (s);
  s ={v} {CLOBBER};
  return;
                                                                                                                      
}

during expand_used_vars, gcc decide that "s" is OK reside in register pair
(regLow, regHigh), instead of on stack.

thus later, MEM[(U * {ref-all})&s + 7B] expanded into:

  regHigh + 3B which means regHigh[31:24],

so "MEM[(U * {ref-all})&s + 7B] = _2;" expanded into

  regHigh[31:24] = _2

then, store_bit_field_1 will get a to_rtx be regHigh, bitsize be 16, and bitnum be 24, so the
8bit outside of regB is safe to be ignored.

I think if store_bit_field_using_insv is called with op0 be a REG_P, and bitsize + bitnum
overflow the reg size, then it's caused by a memory object optimized into register
object.

As the outer code decided it's OK to fit the mem obj into a register, all those bit out of
reg size should be safe to ignore.

the following code in store_bit_field_using_insv haven't consider above MEM->REG situation,
it always assume bitnum + bitsize is within unit which is wrong.
 
  if (BITS_BIG_ENDIAN != BYTES_BIG_ENDIAN)
    bitnum = unit - bitsize - bitnum;

while my patch do have a problem, I should restrict the check on REG_P/SUBREG_P (op0) only.
so is the patch OK with on extra check

  if (REG_P (xop0) || (SUBREG_P (xop0) && REG_P (SUBREG_REG (xop0))))

thanks

Regards,
Jiong

> jeff