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

[Jiong Wang]

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