[Bug target/59401] New: [SH] GBR addressing mode optimization produces wrong code

[Bug target/59401] New: [SH] GBR addressing mode optimization produces wrong code

[olegendo at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59401

            Bug ID: 59401
           Summary: [SH] GBR addressing mode optimization produces wrong
                    code
           Product: gcc
           Version: 4.9.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: olegendo at gcc dot gnu.org
            Target: sh*-*-*

The GBR addressing mode optimization which was added in 4.8 is buggy.
The following example:

struct tcb_t
{
  int x, y, z, w;
};

int test_00 (int a, tcb_t* b)
{
  tcb_t* tcb = (a & 5) ? (tcb_t*)__builtin_thread_pointer () : b;

  return tcb->w + tcb->x;
}

compiled with -O2 results in:

        mov.l   @(12,gbr),r0
        mov     r0,r2
        mov.l   @(0,gbr),r0
        rts
        add     r2,r0

which is obviously wrong code.  This is because sh_find_base_reg_disp in sh.c
will step insns outside the current basic block without any further
considerations.  This is only OK to do if the predecessor basic block has a
fall through edge to the current basic block (i.e. there are no labels in
between).  Otherwise the address reg in question might be set in multiple basic
blocks which must be analyzed.
In the above test case GBR addressing modes can't be used actually.

[Bug target/59401] [SH] GBR addressing mode optimization produces wrong code

[olegendo at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59401

--- Comment #1 from Oleg Endo <olegendo at gcc dot gnu.org> ---
An example where the base address is retrieved from the GBR in one basic block,
but used in different basic blocks:

struct tcb_t
{
  int x, y, z, w;
};

int test_01 (int a, tcb_t* b, int c)
{
  tcb_t* tcb = (tcb_t*)__builtin_thread_pointer ();

  return (a & 5) ? tcb->x : tcb->w;
}

By coincidence it produces correct code:

        mov     r4,r0
        tst     #5,r0
        bf      .L7
        mov.l   @(12,gbr),r0
        rts
        nop
.L7:
        rts
        mov.l   @(0,gbr),r0

A proper fix for this would be to collect all "leaf values" for the address
registers in question from all predecessor basic blocks as it's done in the
function sh_optimize_sett_clrt::find_last_ccreg_values in
sh_optimize_sett_clrt.cc.

[Bug target/59401] [SH] GBR addressing mode optimization produces wrong code

[olegendo at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=59401

--- Comment #2 from Oleg Endo <olegendo at gcc dot gnu.org> ---
Author: olegendo
Date: Sun Oct 12 23:14:07 2014
New Revision: 216128

URL: https://gcc.gnu.org/viewcvs?rev=216128&root=gcc&view=rev
Log:
gcc/
    PR target/59401
    * config/sh/sh-protos (sh_find_equiv_gbr_addr): Use rtx_insn* instead
    of rtx.
    * config/sh/sh.c (sh_find_equiv_gbr_addr): Use def chains instead of
    insn walking.
    (sh_find_equiv_gbr_addr): Do nothing if input mem is already a GBR
    address.  Use def chains to handle GBR clobbering call insns.

gcc/testsuite/
    PR target/59401
    PR target/54760
    * gcc.target/pr54760-5.c: New.
    * gcc.target/pr54760-6.c: New.
    * gcc.target/sh/pr59401-1.c: New.

Added:
    trunk/gcc/testsuite/gcc.target/sh/pr54760-5.c
    trunk/gcc/testsuite/gcc.target/sh/pr54760-6.c
    trunk/gcc/testsuite/gcc.target/sh/pr59401-1.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/sh/sh-protos.h
    trunk/gcc/config/sh/sh.c
    trunk/gcc/testsuite/ChangeLog

[Bug target/59401] [SH] GBR addressing mode optimization produces wrong code

[olegendo at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=59401

Oleg Endo <olegendo at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |kkojima at gcc dot gnu.org

--- Comment #3 from Oleg Endo <olegendo at gcc dot gnu.org> ---
Kaz, I'd like to backport this to 4.8 and 4.9, but I can't decide in which
form.

The patch from r216128 could be applied to the branches.  Another option is to
prevent the sh_find_equiv_gbr_addr function from crossing labels.  This is just
a few lines, but of course it produces not so good results in some cases.

Do you have an opinion on that?

[Bug target/59401] [SH] GBR addressing mode optimization produces wrong code

[kkojima at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=59401

--- Comment #4 from Kazumoto Kojima <kkojima at gcc dot gnu.org> ---
(In reply to Oleg Endo from comment #3)
> Do you have an opinion on that?

Looks the latter is enough for the released branches.  I'm OK with eather way,
though.

[Bug target/59401] [SH] GBR addressing mode optimization produces wrong code

[olegendo at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=59401

--- Comment #5 from Oleg Endo <olegendo at gcc dot gnu.org> ---
Ugh, there's actually another problem with this thing, I think:

void other (void);

int test0 (void)
{
  int x = ((int*)__builtin_thread_pointer ())[2];

  other ();

  return ((int*)__builtin_thread_pointer ())[5] + x;
}

compiles to:
    stc    gbr,r8
    mov.l    .L3,r1
    jsr    @r1
    mov.l    @(8,r8),r9

    mov.l    @(20,r8),r0  << use GBR value before the call
    add    r9,r0
    lds.l    @r15+,pr
    mov.l    @r15+,r9

    rts
    mov.l    @r15+,r8

By default, GBR is marked as call-clobbered.  Currently, if the GBR mem
optimization thing sees any calls in a function it will not form the GBR
address.  It's not optimal, but was supposed to produce at least correct code. 
Obviously the code above is wrong.  The __builtin_thread_pointer insn is
hoisted by something else before combine/split1.  The patch from r216128 is
incomplete.  I'm checking it out.

[Bug target/59401] [SH] GBR addressing mode optimization produces wrong code

[olegendo at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=59401

--- Comment #6 from Oleg Endo <olegendo at gcc dot gnu.org> ---
(In reply to Oleg Endo from comment #5)
> Ugh, there's actually another problem with this thing, I think:
> 
> ...
> 
> By default, GBR is marked as call-clobbered.  Currently, if the GBR mem
> optimization thing sees any calls in a function it will not form the GBR
> address.  It's not optimal, but was supposed to produce at least correct
> code.  Obviously the code above is wrong.  The __builtin_thread_pointer insn
> is hoisted by something else before combine/split1.  The patch from r216128
> is incomplete.  I'm checking it out.

No, hoisting __builtin_thread_pointer is OK.
It only makes the following impossible to do:

void foo (void* otherthread)
{
  ...
  access current thread's context
  ...

  switch_thread (otherthread);

  ...
  access otherthread's context
  ...
}

The compiler doesn't know that switch_thread modifies the thread pointer.  One
workaround is:

  switch_thread (otherthread);
  void* newtp;
  __asm__ __volatile__ ("stc gbr, %0" : "=r" (newtp));
  __builtin_set_thread_pointer (newtp);

However, this is just a hypothetical use case.  I don't think there is an
immediate use for that.  Usually the thread pointer is not switched like that
in the middle of a function, probably not even when implementing cooperative
threading/fibers.

Thus I think it would make sense to change the default behavior from GBR call
clobbered to GBR call preserved.  Actually making GBR call preserved is the
only way for the GBR mem access optimization to make any sense because
otherwise it will never form GBR mems if calls are present in a function, which
defeats its purpose.  It would be possible to make a better analysis of when
GBR is used (before / after calls etc), but I think it's worth it in this case.


Kaz, what's your opinion on making GBR to be call preserved by default?

[Bug target/59401] [SH] GBR addressing mode optimization produces wrong code

[kkojima at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=59401

--- Comment #7 from Kazumoto Kojima <kkojima at gcc dot gnu.org> ---
(In reply to Oleg Endo from comment #6)
> Kaz, what's your opinion on making GBR to be call preserved by default?

Looks OK to me for 5.0.  It's clearly an ABI change but a change to
the more robust direction and wouldn't be surprising to users.

[Bug target/59401] [SH] GBR addressing mode optimization produces wrong code

[olegendo at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=59401

--- Comment #8 from Oleg Endo <olegendo at gcc dot gnu.org> ---
(In reply to Kazumoto Kojima from comment #7)
> (In reply to Oleg Endo from comment #6)
> > Kaz, what's your opinion on making GBR to be call preserved by default?
> 
> Looks OK to me for 5.0.  It's clearly an ABI change but a change to
> the more robust direction and wouldn't be surprising to users.

Yes, I was thinking to do that for 5.0, not for the released branches.  Just to
be on the safe side, for your next test run, could you please change the value
for gbr in sh.h CALL_USED_REGISTERS from '1' to '0' and confirm that everything
is still OK?

[Bug target/59401] [SH] GBR addressing mode optimization produces wrong code

[kkojima at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=59401

--- Comment #9 from Kazumoto Kojima <kkojima at gcc dot gnu.org> ---
(In reply to Oleg Endo from comment #8)
> change the
> value for gbr in sh.h CALL_USED_REGISTERS from '1' to '0' and confirm that
> everything is still OK?

The comment and document about CALL_USED_REGISTERS say that it must be
a superset of FIXED_REGISTERS.  CALL_REALLY_USED_REGISTERS might be
a macro for that purpose.

[Bug target/59401] [SH] GBR addressing mode optimization produces wrong code

[olegendo at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=59401

--- Comment #10 from Oleg Endo <olegendo at gcc dot gnu.org> ---
(In reply to Kazumoto Kojima from comment #9)
> (In reply to Oleg Endo from comment #8)
> > change the
> > value for gbr in sh.h CALL_USED_REGISTERS from '1' to '0' and confirm that
> > everything is still OK?
> 
> The comment and document about CALL_USED_REGISTERS say that it must be
> a superset of FIXED_REGISTERS.  CALL_REALLY_USED_REGISTERS might be
> a macro for that purpose.

Right.  In this case it's probably better to do it in
sh_conditional_register_usage.  It would be nice if '-fcall-saved-gbr' and
'-fcall-used-gbr' still remained functional afterwards ... I'll give it a try.

[Bug target/59401] [SH] GBR addressing mode optimization produces wrong code

[olegendo at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=59401

--- Comment #11 from Oleg Endo <olegendo at gcc dot gnu.org> ---
Created attachment 33723
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=33723&action=edit
Make GBR call preserved by default

(In reply to Oleg Endo from comment #10)
> (In reply to Kazumoto Kojima from comment #9)
> > (In reply to Oleg Endo from comment #8)
> > > change the
> > > value for gbr in sh.h CALL_USED_REGISTERS from '1' to '0' and confirm that
> > > everything is still OK?
> > 
> > The comment and document about CALL_USED_REGISTERS say that it must be
> > a superset of FIXED_REGISTERS.  CALL_REALLY_USED_REGISTERS might be
> > a macro for that purpose.
> 
> Right.  In this case it's probably better to do it in
> sh_conditional_register_usage.  It would be nice if '-fcall-saved-gbr' and
> '-fcall-used-gbr' still remained functional afterwards ... I'll give it a
> try.

sh_conditional_register_usage is invoked after the -fcall-*-* options are
processed.  Defining the CALL_REALLY_USED_REGISTERS macro with the default
values works as expected, i.e. -fcall-used-gbr still works.

Kaz, could you please add the attached patch to your test run?

[Bug target/59401] [SH] GBR addressing mode optimization produces wrong code

[olegendo at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=59401

--- Comment #13 from Oleg Endo <olegendo at gcc dot gnu.org> ---
Author: olegendo
Date: Thu Oct 16 12:21:29 2014
New Revision: 216314

URL: https://gcc.gnu.org/viewcvs?rev=216314&root=gcc&view=rev
Log:
gcc/
    PR target/59401
    * config/sh/sh.h (CALL_REALLY_USED_REGISTERS): Expand macro and set
    GBR to 0.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/sh/sh.h