Re: patch for elimination to SP when it is changed in RTL (PR57293)

[Yvan Roux <yvan dot roux at linaro dot org>]

Thanks for your help Vlad.  Another bad news about this PR fix, is
that it has resurrected the thumb_movhi_clobber bug (PR 58785) but in
a different manner as the original failing testcase still pass.  I
attached a testcase to be compiled with :

cc1 -mthumb -mcpu=cortex-m0 -O2 m.c

And Thumb bootstrap seems to be broken with an ICE in check_rtl, I'm
checking if it is the same issue.

Yvan


On 12 December 2013 20:18, Vladimir Makarov <vmakarov@redhat.com> wrote:
> On 12/11/2013, 1:59 PM, Yvan Roux wrote:
>>
>> On 11 December 2013 19:25, Vladimir Makarov <vmakarov@redhat.com> wrote:
>>>
>>> On 12/11/2013, 5:35 AM, Yvan Roux wrote:
>>>>
>>>>
>>>> Hi Vladimir,
>>>>
>>>> I've some regressions on ARM after this SP elimination patch, and they
>>>> are execution failures.  Here is the list:
>>>>
>>>> g++.dg/cilk-plus/AN/array_test_ND_tplt.cc  -O3 -fcilkplus
>>>> gcc.c-torture/execute/va-arg-22.c  -O2
>>>> gcc.dg/atomic/c11-atomic-exec-5.c  -O0
>>>> gfortran.dg/direct_io_12.f90  -O[23]
>>>> gfortran.dg/elemental_dependency_1.f90  -O2
>>>> gfortran.dg/matmul_2.f90  -O2
>>>> gfortran.dg/matmul_6.f90  -O2
>>>> gfortran.dg/mvbits_7.f90  -O3
>>>> gfortran.dg/unlimited_polymorphic_1.f03  -O3
>>>>
>>>> I reduced and looked at var-arg-22.c and the issue is that in
>>>> lra_eliminate_regs_1  (called by get_equiv_with_elimination) we
>>>> transformed sfp + 0x4c in sp + 0xfc because of a bad sp offset.  What
>>>> we try to do here is to change the pseudo 195 of the insn 118 below :
>>>>
>>>> (insn 118 114 112 8 (set (reg:DI 195)
>>>>           (unspec:DI [
>>>>                   (mem:DI (plus:SI (reg/f:SI 215)
>>>>                           (const_int 8 [0x8])) [7 MEM[(struct A35 *)_12
>>>> + 64B]+8 S8 A8])
>>>>               ] UNSPEC_UNALIGNED_LOAD)) v2.c:49 146 {unaligned_loaddi}
>>>>        (expr_list:REG_EQUIV (mem/c:DI (plus:SI (reg/f:SI 192)
>>>>                   (const_int 8 [0x8])) [7 a35+8 S8 A32])
>>>>           (nil)))
>>>>
>>>> with its equivalent (x arg of lra_eliminate_regs_1):
>>>>
>>>> (mem/c:DI (plus:SI (reg/f:SI 102 sfp)
>>>>           (const_int 76 [0x4c])) [7 a35+8 S8 A32])
>>>>
>>>> lra_eliminate_regs_1 is called with full_p = true (it is not really
>>>> clear for what it means),
>>>
>>>
>>>
>>> It means we use full offset between the regs, otherwise we use change in
>>> the
>>> full offset from the previous iteration (it can be changed as we reserve
>>> stack memory for spilled pseudos and the reservation can be done several
>>> times).  As equiv value is stored as it was before any elimination, we
>>> need
>>> always to use full offset to make elimination.
>>
>>
>> Ok thanks it's clearer.
>>
>>>   but in the PLUS switch case, we have offset
>>>>
>>>>
>>>> = 0xb (given by ep->offset) and  as lra_get_insn_recog_data
>>>> (insn)->sp_offset value is 0, we will indeed add 0xb to the original
>>>> 0x4c offset.
>>>>
>>>
>>> 0 value is suspicious because it is default.  We might have not set up it
>>> from neighbor insns.
>>>
>>>
>>>
>>>> So, here I don't get if it is the sp_offset value of the
>>>> lra_insn_recog_data element which is not well updated or if lra_
>>>> eliminate_regs_1 has to be called with update_p and not full_p (which
>>>> fixed the value in that particular case).  Is it more obvious for you
>>>> ?
>>>>
>>>
>>> Yvan, could you send me the reduced preprocessed case and the options for
>>> cc1 to reproduce it.
>>
>>
>>
>> Here is cc1 command line :
>>
>> cc1 -quiet -march=armv7-a -mtune=cortex-a15 -mfloat-abi=hard
>> -mfpu=neon -mthumb  v2.c  -O2
>>
>> I use a native build on a chromebook, but it's reproducible with a
>> cross compiler.
>>
>> With the attached test case the issue is when processing insn 118.
>
>
> The offset is updated two times and that is wrong.  That is because memory
> in init insn is shared by ira_reg_equiv and the test involves 2 equivalent
> substitutions.  As I wrote equiv should be stored in original form by the
> current patch design.  Simple copying will not work as the first
> substitution is not done in this case.
>
> I need some time to think how to fix it better still I'll try to fix it
> tomorrow.  I expected that the patch might have some problems.  The patch
> code is quite big although it is just a long standing PR fix. Therefore that
> was my first PR fixed on stage 3.  It is good to have it tested earlier and
> sorry to break some arm tests.
>
>

void free (void *) ;

extern int *__errno (void);

typedef unsigned int size_t;


typedef unsigned int __uint32_t;
typedef unsigned short __uint16_t;

typedef struct _bufhead BUFHEAD;

struct _bufhead {
 BUFHEAD *prev;
 BUFHEAD *next;
 BUFHEAD *ovfl;
 __uint32_t addr;
 char *page;
 char flags;
};

typedef BUFHEAD **SEGMENT;

typedef struct hashhdr {
 int magic;
 int version;
 __uint32_t lorder;
 int bsize;
 int bshift;
 int dsize;
 int ssize;
 int sshift;
 int ovfl_point;

 int last_freed;
 int max_bucket;
 int high_mask;
 int low_mask;

 int ffactor;
 int nkeys;
 int hdrpages;
 int h_charkey;


 int spares[32];
 __uint16_t bitmaps[32];

} HASHHDR;

typedef struct htab {
 HASHHDR hdr;
 int nsegs;
 int exsegs;

 __uint32_t
     (*hash)(const void *, size_t);
 int flags;
 int fp;
 char *tmp_buf;
 char *tmp_key;
 BUFHEAD *cpage;
 int cbucket;
 int cndx;
 int error;

 int new_file;

 int save_file;


 __uint32_t *mapp[32];
 int nmaps;
 int nbufs;

 BUFHEAD bufhead;
 SEGMENT *dir;
} HTAB;


extern int
collect_data(hashp, bufp, len, set)
 HTAB *hashp;
 BUFHEAD *bufp;
 int len, set;
{
 __uint16_t *bp;
 char *p;
 BUFHEAD *xbp;
 __uint16_t save_addr;
 int mylen, totlen;

 p = bufp->page;
 bp = (__uint16_t *)p;
 mylen = hashp->hdr.bsize - bp[1];
 save_addr = bufp->addr;

 if (bp[2] == 3) {
  totlen = len + mylen;
  if (hashp->tmp_buf)
   free(hashp->tmp_buf);
  if ((hashp->tmp_buf = (char *)malloc(totlen)) == ((void *)0))
   return (-1);
  if (set) {
   hashp->cndx = 1;
   if (bp[0] == 2) {
    hashp->cpage = ((void *)0);
    hashp->cbucket++;
   } else {
    hashp->cpage =
        __get_buf(hashp, bp[bp[0] - 1], bufp, 0);
    if (!hashp->cpage)
     return (-1);
    else if (!((__uint16_t *)hashp->cpage->page)[0]) {
     hashp->cbucket++;
     hashp->cpage = ((void *)0);
    }
   }
  } 
 } else {
  xbp = __get_buf(hashp, bp[bp[0] - 1], bufp, 0);
  if (!xbp || ((totlen =
      collect_data(hashp, xbp, len + mylen, set)) < 1))
   return (-1);
 }
 if (bufp->addr != save_addr) {
  return (-1);
 }
 return (totlen);
}