[Bug rtl-optimization/64895] New: [5 Regression] RA picks the wrong register for -fipa-ra

[Bug rtl-optimization/64895] New: [5 Regression] RA picks the wrong register for -fipa-ra

[hjl.tools at gmail dot com]

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

            Bug ID: 64895
           Summary: [5 Regression] RA picks the wrong register for
                    -fipa-ra
           Product: gcc
           Version: 5.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: rtl-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: hjl.tools at gmail dot com
                CC: vmakarov at redhat dot com

'-fipa-ra'
     Use caller save registers for allocation if those registers are not
     used by any called function.  In that case it is not necessary to
     save and restore them around calls.  This is only possible if
     called functions are part of same compilation unit as current
     function and they are compiled before it.

But in this case testcase 
[hjl@gnu-tools-1 gcc]$ cat /tmp/x.c 
static int __attribute__((noinline))
bar (int x)
{
  if (x > 4)
    return bar (x - 3);
  return 0;
}

int __attribute__((noinline))
foo (int y)
{
  return y + bar (y);
}
[hjl@gnu-tools-1 gcc]$ ./xgcc -B./ -m32 -fpic -O2 -fipa-ra -fomit-frame-pointer
-fno-optimize-sibling-calls -mregparm=1 /tmp/x.c -S
-fno-asynchronous-unwind-tables        
[hjl@gnu-tools-1 gcc]$ cat x.s 
    .file    "x.c"
    .section    .text.unlikely,"ax",@progbits
.LCOLDB0:
    .text
.LHOTB0:
    .p2align 4,,15
    .type    bar, @function
bar:
    cmpl    $4, %eax
    jg    .L7
    xorl    %eax, %eax
    ret
    .p2align 4,,10
    .p2align 3
.L7:
    subl    $12, %esp
    subl    $3, %eax
    call    bar
    addl    $12, %esp
    ret
    .size    bar, .-bar
    .section    .text.unlikely
.LCOLDE0:
    .text
.LHOTE0:
    .section    .text.unlikely
.LCOLDB1:
    .text
.LHOTB1:
    .p2align 4,,15
    .globl    foo
    .type    foo, @function
foo:
    pushl    %ebx
    movl    %eax, %ebx
    subl    $8, %esp
    call    bar
    addl    $8, %esp
    addl    %ebx, %eax
    popl    %ebx
    ret
    .size    foo, .-foo
    .section    .text.unlikely
.LCOLDE1:
    .text
.LHOTE1:
    .ident    "GCC: (GNU) 5.0.0 20150201 (experimental)"
    .section    .note.GNU-stack,"",@progbits
[hjl@gnu-tools-1 gcc]$ 

Pick EBX instead of EDX to save EAX in foo.

[Bug rtl-optimization/64895] [5 Regression] RA picks the wrong register for -fipa-ra

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

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

--- Comment #1 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
How could this be a regression?  -fipa-ra is a new option, I believe your
testcase isn't a wrong-code, but an enhancement request for a new feature.

[Bug rtl-optimization/64895] [5 Regression] RA picks the wrong register for -fipa-ra

[hjl.tools at gmail dot com]

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

--- Comment #2 from H.J. Lu <hjl.tools at gmail dot com> ---
(In reply to Jakub Jelinek from comment #1)
> How could this be a regression?  -fipa-ra is a new option, I believe your
> testcase isn't a wrong-code, but an enhancement request for a new feature.

It is a regression from when -fipa-ra was implemented, probably triggered by
r216154.

[Bug rtl-optimization/64895] [5 Regression] RA picks the wrong register for -fipa-ra

[jakub at gcc dot gnu.org]

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

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
That doesn't count, regressions for release management purposes are only
regressions from released compilers.

[Bug rtl-optimization/64895] [5 Regression] RA picks the wrong register for -fipa-ra

[hjl.tools at gmail dot com]

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

--- Comment #4 from H.J. Lu <hjl.tools at gmail dot com> ---
From

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64342#c9

The test fails due to different and orthogonal issue: RA chooses %ebx for a
temporary when -fpic is used:

        movl    %eax, %ebx
        call    bar
        addl    %ebx, %eax

The push and pop insns are from prologue/epilogue since call-saved reg is used
(%ebx), not due to save/restore the reg around the call:

(note 3 4 19 2 NOTE_INSN_FUNCTION_BEG)
(insn/f:TI 19 3 20 2 (set (mem:SI (pre_dec:SI (reg/f:SI 7 sp)) [0  S4 A8])
        (reg:SI 3 bx)) fuse-caller-save.c:16 66 {*pushsi2}
     (expr_list:REG_DEAD (reg:SI 3 bx)
        (nil)))
(note 20 19 2 2 NOTE_INSN_PROLOGUE_END)
...
(note 27 15 24 2 NOTE_INSN_EPILOGUE_BEG)
(insn/f:TI 24 27 25 2 (set (reg:SI 3 bx)
        (mem:SI (post_inc:SI (reg/f:SI 7 sp)) [0  S4 A8]))
fuse-caller-save.c:18 74 {*popsi1}
     (expr_list:REG_CFA_ADJUST_CFA (set (reg/f:SI 7 sp)
            (plus:SI (reg/f:SI 7 sp)
                (const_int 4 [0x4])))
        (expr_list:REG_CFA_RESTORE (reg:SI 3 bx)
            (nil))))

So, please open a new PR due to RA issue: since %edx was allocated for PIC
register (but later removed), another call-used (%ecx) should be used here.

[Bug rtl-optimization/64895] RA picks the wrong register for -fipa-ra

[ubizjak at gmail dot com]

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

Uroš Bizjak <ubizjak at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2015-03-06
                 CC|                            |law at gcc dot gnu.org
     Ever confirmed|0                           |1

--- Comment #5 from Uroš Bizjak <ubizjak at gmail dot com> ---
Confirmed, still happens with r221230 even after PR64342 fix:

The testcase from the Comment #0, compiled with:

-m32 -fpic -O2 -fipa-ra -fomit-frame-pointer -fno-optimize-sibling-calls
-mregparm=1 -fno-asynchronous-unwind-tables:

compiles to:

foo:
        pushl   %ebx
        movl    %eax, %ebx
        subl    $8, %esp
        call    bar
        addl    $8, %esp
        addl    %ebx, %eax
        popl    %ebx
        ret
>From gcc-bugs-return-479541-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org Fri Mar 06 08:25:55 2015
Return-Path: <gcc-bugs-return-479541-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org>
Delivered-To: listarch-gcc-bugs@gcc.gnu.org
Received: (qmail 6878 invoked by alias); 6 Mar 2015 08:25:55 -0000
Mailing-List: contact gcc-bugs-help@gcc.gnu.org; run by ezmlm
Precedence: bulk
List-Id: <gcc-bugs.gcc.gnu.org>
List-Archive: <http://gcc.gnu.org/ml/gcc-bugs/>
List-Post: <mailto:gcc-bugs@gcc.gnu.org>
List-Help: <mailto:gcc-bugs-help@gcc.gnu.org>
Sender: gcc-bugs-owner@gcc.gnu.org
Delivered-To: mailing list gcc-bugs@gcc.gnu.org
Received: (qmail 6833 invoked by uid 48); 6 Mar 2015 08:25:51 -0000
From: "trippels at gcc dot gnu.org" <gcc-bugzilla@gcc.gnu.org>
To: gcc-bugs@gcc.gnu.org
Subject: [Bug lto/65316] [5 Regression] LTO: Uninitialized memory / ICE with -g -fno-lto-odr-type-merging: in types_same_for_odr, at ipa-devirt.c:465
Date: Fri, 06 Mar 2015 08:25:00 -0000
X-Bugzilla-Reason: CC
X-Bugzilla-Type: changed
X-Bugzilla-Watch-Reason: None
X-Bugzilla-Product: gcc
X-Bugzilla-Component: lto
X-Bugzilla-Version: 5.0
X-Bugzilla-Keywords: ice-on-valid-code
X-Bugzilla-Severity: normal
X-Bugzilla-Who: trippels at gcc dot gnu.org
X-Bugzilla-Status: ASSIGNED
X-Bugzilla-Priority: P1
X-Bugzilla-Assigned-To: hubicka at gcc dot gnu.org
X-Bugzilla-Target-Milestone: 5.0
X-Bugzilla-Flags:
X-Bugzilla-Changed-Fields:
Message-ID: <bug-65316-4-Q2X497Tr5r@http.gcc.gnu.org/bugzilla/>
In-Reply-To: <bug-65316-4@http.gcc.gnu.org/bugzilla/>
References: <bug-65316-4@http.gcc.gnu.org/bugzilla/>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
X-Bugzilla-URL: http://gcc.gnu.org/bugzilla/
Auto-Submitted: auto-generated
MIME-Version: 1.0
X-SW-Source: 2015-03/txt/msg00685.txt.bz2
Content-length: 248

https://gcc.gnu.org/bugzilla/show_bug.cgi?ide316

--- Comment #4 from Markus Trippelsdorf <trippels at gcc dot gnu.org> ---
I'm not sure that uninitialized memory is the cause of this bug.
Looks more like a bug in the hash-table implementation.

[Bug rtl-optimization/64895] RA picks the wrong register for -fipa-ra

[vries at gcc dot gnu.org]

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

--- Comment #6 from vries at gcc dot gnu.org ---
>From PR64342 comment 7:

Register allocation seems to progress similarly, up until this message in
reload, which seems to be directly related to the r216154 patch:
...
    Spill r86 after risky transformations
  Reassigning non-reload pseudos
           Assign 3 to r86 (freq=3000)
...

I've looked a bit in more detail at this message.

fipa-ra manages to allocate hardreg 1 (dx) to the call-crossing liverange as
before. But setup_live_pseudos_and_spill_after_risky_transforms looks at the
conflict regs, and finds that lra_reg_info[regno].conflict_hard_regs contains
dx. Hence, it spills the liverange into an available reg, which happens to be
hardreg 3 (bx).

So, the risky transformations and fipa-ra have in common that they allocate
registers from the conflict_hard_regs set. In the case of -fipa-ra, we don't
want setup_live_pseudos_and_spill_after_risky_transforms to undo the -fipa-ra
allocation, but currently there's no way to distinguish between the two.

Using this patch, we make sure the conflict registers don't include dx for this
testcase, and the test-cases passes again:
...
diff --git a/gcc/lra-lives.c b/gcc/lra-lives.c
index 9dfffb6..0231057 100644
--- a/gcc/lra-lives.c
+++ b/gcc/lra-lives.c
@@ -636,8 +636,11 @@ check_pseudos_live_through_calls (int regno)
   if (! sparseset_bit_p (pseudos_live_through_calls, regno))
     return;
   sparseset_clear_bit (pseudos_live_through_calls, regno);
+
   IOR_HARD_REG_SET (lra_reg_info[regno].conflict_hard_regs,
-                   call_used_reg_set);
+                   !hard_reg_set_empty_p
(lra_reg_info[regno].actual_call_used_reg_set)
+                   ? lra_reg_info[regno].actual_call_used_reg_set
+                   : call_used_reg_set);

   for (hr = 0; hr < FIRST_PSEUDO_REGISTER; hr++)
     if (HARD_REGNO_CALL_PART_CLOBBERED (hr, PSEUDO_REGNO_MODE (regno)))
...

[Bug rtl-optimization/64895] RA picks the wrong register for -fipa-ra

[vmakarov at gcc dot gnu.org]

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

Vladimir Makarov <vmakarov at gcc dot gnu.org> changed:

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

--- Comment #7 from Vladimir Makarov <vmakarov at gcc dot gnu.org> ---
(In reply to vries from comment #6)
> From PR64342 comment 7:
> 
> Register allocation seems to progress similarly, up until this message in
> reload, which seems to be directly related to the r216154 patch:
> ...
>     Spill r86 after risky transformations
>   Reassigning non-reload pseudos
>            Assign 3 to r86 (freq=3000)
> ...
> 
> I've looked a bit in more detail at this message.
> 
> fipa-ra manages to allocate hardreg 1 (dx) to the call-crossing liverange as
> before. But setup_live_pseudos_and_spill_after_risky_transforms looks at the
> conflict regs, and finds that lra_reg_info[regno].conflict_hard_regs
> contains dx. Hence, it spills the liverange into an available reg, which
> happens to be hardreg 3 (bx).
> 
> So, the risky transformations and fipa-ra have in common that they allocate
> registers from the conflict_hard_regs set. In the case of -fipa-ra, we don't
> want setup_live_pseudos_and_spill_after_risky_transforms to undo the
> -fipa-ra allocation, but currently there's no way to distinguish between the
> two.
> 

Right.  Introduction of pic pseudo and possible rematerializations of memory
references with the pic pseudo made checking the new conflict first necessary. 
Before this it was not a problem as pic hard reg was used only for pic
addressing.

> Using this patch, we make sure the conflict registers don't include dx for
> this testcase, and the test-cases passes again:
> ...
> diff --git a/gcc/lra-lives.c b/gcc/lra-lives.c
> index 9dfffb6..0231057 100644
> --- a/gcc/lra-lives.c
> +++ b/gcc/lra-lives.c
> @@ -636,8 +636,11 @@ check_pseudos_live_through_calls (int regno)
>    if (! sparseset_bit_p (pseudos_live_through_calls, regno))
>      return;
>    sparseset_clear_bit (pseudos_live_through_calls, regno);
> +
>    IOR_HARD_REG_SET (lra_reg_info[regno].conflict_hard_regs,
> -                   call_used_reg_set);
> +                   !hard_reg_set_empty_p
> (lra_reg_info[regno].actual_call_used_reg_set)
> +                   ? lra_reg_info[regno].actual_call_used_reg_set
> +                   : call_used_reg_set);
>  
>    for (hr = 0; hr < FIRST_PSEUDO_REGISTER; hr++)
>      if (HARD_REGNO_CALL_PART_CLOBBERED (hr, PSEUDO_REGNO_MODE (regno)))
> ...

The patch looks ok to me.  Tom, could you prepare the patch (check it mostly
for x86-64 bootstrap and testsuite) and commit it to the trunk.  I approve it.

[Bug rtl-optimization/64895] RA picks the wrong register for -fipa-ra

[vries at gcc dot gnu.org]

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

vries at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED
           Assignee|unassigned at gcc dot gnu.org      |vries at gcc dot gnu.org

--- Comment #8 from vries at gcc dot gnu.org ---
Author: vries
Date: Thu Mar 12 06:59:34 2015
New Revision: 221372

URL: https://gcc.gnu.org/viewcvs?rev=221372&root=gcc&view=rev
Log:
Use actual_call_used_reg_set to find conflicting regs

2015-03-12  Tom de Vries  <tom@codesourcery.com>

    * lra-lives.c (check_pseudos_live_through_calls): Use
    actual_call_used_reg_set instead of call_used_reg_set, if available.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/lra-lives.c

[Bug rtl-optimization/64895] RA picks the wrong register for -fipa-ra

[vries at gcc dot gnu.org]

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

--- Comment #9 from vries at gcc dot gnu.org ---
Author: vries
Date: Thu Mar 12 08:01:24 2015
New Revision: 221374

URL: https://gcc.gnu.org/viewcvs?rev=221374&root=gcc&view=rev
Log:
Revert 'require nonpic target' for fuse-caller-save*.c

2015-03-12  Tom de Vries  <tom@codesourcery.com>

    PR rtl-optimization/64895
    * gcc.target/i386/fuse-caller-save-rec.c: Revert require nonpic target.
    * gcc.target/i386/fuse-caller-save-xmm.c: Ditto.
    * gcc.target/i386/fuse-caller-save.c: Ditto.

Modified:
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/gcc.target/i386/fuse-caller-save-rec.c
    trunk/gcc/testsuite/gcc.target/i386/fuse-caller-save-xmm.c
    trunk/gcc/testsuite/gcc.target/i386/fuse-caller-save.c

[Bug rtl-optimization/64895] RA picks the wrong register for -fipa-ra

[vries at gcc dot gnu.org]

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

--- Comment #10 from vries at gcc dot gnu.org ---
Author: vries
Date: Mon Mar 16 09:42:21 2015
New Revision: 221448

URL: https://gcc.gnu.org/viewcvs?rev=221448&root=gcc&view=rev
Log:
Revert 'Use actual_call_used_reg_set to find conflicting regs'

2015-03-16  Tom de Vries  <tom@codesourcery.com>

    PR middle-end/65414
    Revert:
    2015-03-12  Tom de Vries  <tom@codesourcery.com>

    PR rtl-optimization/64895
    * lra-lives.c (check_pseudos_live_through_calls): Use
    actual_call_used_reg_set instead of call_used_reg_set, if available.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/lra-lives.c

[Bug rtl-optimization/64895] RA picks the wrong register for -fipa-ra

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |REOPENED
         Resolution|FIXED                       |---

--- Comment #11 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Patch has been reverted.