Bug 78617 - LRA clobbers live register during rematerialization
Summary: LRA clobbers live register during rematerialization
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: rtl-optimization (show other bugs)
Version: 6.2.1
: P3 normal
Target Milestone: 5.5
Assignee: Not yet assigned to anyone
URL:
Keywords: wrong-code
Depends on:
Blocks:
 
Reported: 2016-11-30 17:16 UTC by Thomas Preud'homme
Modified: 2017-01-17 10:24 UTC (History)
1 user (show)

See Also:
Host:
Target: arm-none-eabi
Build:
Known to work: 5.4.1, 6.3.1
Known to fail: 5.4.0, 6.3.0
Last reconfirmed: 2016-11-30 00:00:00


Attachments
Testcase showing wrong rematerialization in LRA at -Os for ARMv6-M (190 bytes, text/x-csrc)
2016-11-30 17:16 UTC, Thomas Preud'homme
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Thomas Preud'homme 2016-11-30 17:16:05 UTC
Created attachment 40208 [details]
Testcase showing wrong rematerialization in LRA at -Os for ARMv6-M

Hi,

The attached code prints the wrong value when compiled by GCC 6 with -march=armv6-m -mthumb pr78xxx.c -Os: it prints 0 instead of 1. The issue is in the code generated for fn4 and gets wrong at LRA. See the code after LRA corresponding to bitwise OR, inlined fn2 call and the comparison with !d:

(insn 46 17 18 2 (set (reg:SI 3 r3 [orig:110 D.5240 ] [110])
        (const_int 1 [0x1])) pr78xxx.c:20 744 {*thumb1_movsi_insn}
     (expr_list:REG_EQUAL (const_int 1 [0x1])
        (nil)))
(jump_insn 18 46 19 2 (set (pc)
        (if_then_else (ne (reg:SI 0 r0 [orig:115 D.5240 ] [115])
                (const_int 0 [0]))
            (label_ref:SI 22)
            (pc))) pr78xxx.c:20 753 {cbranchsi4_insn}
     (int_list:REG_BR_PROB 5000 (nil))
 -> 22)
(note 19 18 20 3 [bb 3] NOTE_INSN_BASIC_BLOCK)
(note 20 19 82 3 NOTE_INSN_DELETED)
(insn 82 20 21 3 (set (reg/v:SI 3 r3 [orig:122 p2 ] [122])
        (reg/v:SI 5 r5 [orig:122 p2 ] [122])) pr78xxx.c:20 744 {*thumb1_movsi_insn}
     (nil))
(insn 21 82 22 3 (parallel [
            (set (reg/v:SI 3 r3 [orig:122 p2 ] [122])
                (ne:SI (reg/v:SI 3 r3 [orig:122 p2 ] [122])
                    (const_int 0 [0])))
            (clobber (reg:SI 5 r5 [130]))
        ]) pr78xxx.c:20 764 {*cstoresi_ne0_thumb1_insn}
     (nil))
(code_label 22 21 23 4 10 "" [1 uses])
(note 23 22 24 4 [bb 4] NOTE_INSN_BASIC_BLOCK)
(jump_insn 24 23 25 4 (set (pc)
        (if_then_else (gt (reg/v:SI 7 r7 [orig:121 p1 ] [121])
                (const_int 1 [0x1]))
            (label_ref 26)
            (pc))) pr78xxx.c:12 753 {cbranchsi4_insn}
     (int_list:REG_BR_PROB 6335 (nil))
 -> 26)
(note 25 24 6 5 [bb 5] NOTE_INSN_BASIC_BLOCK)
(insn 6 25 26 5 (set (reg:SI 3 r3 [orig:110 D.5240 ] [110])
        (const_int 0 [0])) pr78xxx.c:12 744 {*thumb1_movsi_insn}
     (nil))
(code_label 26 6 27 6 11 "" [1 uses])
(note 27 26 48 6 [bb 6] NOTE_INSN_BASIC_BLOCK)
(insn 48 27 86 6 (set (reg:SI 0 r0 [orig:132 D.5241 ] [132])
        (const_int 1 [0x1])) pr78xxx.c:20 744 {*thumb1_movsi_insn}
     (expr_list:REG_EQUAL (const_int 1 [0x1])
        (nil)))
(insn 86 48 84 6 (parallel [
            (set (reg:SI 2 r2 [orig:125 D.5241 ] [125])
                (eq:SI (reg:SI 4 r4 [orig:111 D.5240 ] [111])
                    (const_int 0 [0])))
            (clobber (reg:SI 3 r3 [126]))
        ]) pr78xxx.c:20 763 {*cstoresi_eq0_thumb1_insn}
     (nil))
(note 84 86 29 6 NOTE_INSN_DELETED)
(jump_insn 29 84 44 6 (set (pc)
        (if_then_else (gt (reg:SI 2 r2 [orig:125 D.5241 ] [125])
                (reg:SI 3 r3 [orig:110 D.5240 ] [110]))
            (label_ref 31)
            (pc))) pr78xxx.c:20 753 {cbranchsi4_insn}
     (int_list:REG_BR_PROB 5000 (nil))
 -> 31)


We can see that !d is rematerialized in instruction 86 but that rematerialization clobbers r3 which is used by jump instruction 29. Before LRA instruction 86 does not exist and jump instruction 29 uses a computation of !d done early in the function.

Note that GCC 7 works since commit r239421 but only because the code that get fed to LRA is different and thus the bug is not triggered.
Comment 1 ktkachov 2016-11-30 17:21:21 UTC
Confirmed on GCC 5 and 6.
The slightly modified testcase below aborts at -Os but not at -O0.

int a = 0;
int d = 1;
int f = 1;

int fn1() {
  return a || 1 >> a;
}

int fn2(int p1, int p2) {
  return p2 >= 2 ? p1 : p1 >> 1;
}

int fn3(int p1) {
  return d ^ p1;
}

int fn4(int p1, int p2) {
  return fn3(!d > fn2((f = fn1() - 1000) || p2, p1));
}

int main() {
  if (fn4 (0, 0) != 1)
    __builtin_abort ();
  return 0;
}
Comment 2 Thomas Preud'homme 2016-11-30 17:25:05 UTC
I've tracked down the problem to do_remat:

The function scans instruction forward in each basic block and looks for a candidate to use for rematerialization. To check whether the candidate is used, it verify that the candidate does not clobber a register that is live at the point of rematerialization. The function thus keeps track of live registers but the initialization of live registers at the beginning of a basic block loop looks suspicious to me:

REG_SET_TO_HARD_REG_SET (live_hard_regs, df_get_live_out (bb));

I see two problems with that. The first one is that it should use df_get_live_in (bb) since this is a forward scan (as attested by FOR_BB_INSNS (bb, insn) and the way the live_hard_regs updating logic is). The second problem is that REG_SET_TO_HARD_REG_SET will only care about hard registers in the bitmap and will not look at reg_renumber for the pseudo register that are live yet all the logic after that uses get_hard_regs that does consider reg_renumber for pseudo registers.

Fixing both solve the issue with the testcase, I am now running a regression test to see if I've done something foulish that breaks half the testsuite.
Comment 3 Thomas Preud'homme 2016-12-07 17:57:25 UTC
Author: thopre01
Date: Wed Dec  7 17:56:53 2016
New Revision: 243374

URL: https://gcc.gnu.org/viewcvs?rev=243374&root=gcc&view=rev
Log:
2016-12-07  Thomas Preud'homme  <thomas.preudhomme@arm.com>

    gcc/
    PR rtl-optimization/78617
    * lra-remat.c (do_remat): Initialize live_hard_regs from live in
    registers, also setting hard registers mapped to pseudo registers.

    gcc/testsuite/
    PR rtl-optimization/78617
    * gcc.c-torture/execute/pr78617.c: New test.

Added:
    trunk/gcc/testsuite/gcc.c-torture/execute/pr78617.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/lra-remat.c
    trunk/gcc/testsuite/ChangeLog
Comment 4 Thomas Preud'homme 2016-12-07 18:16:24 UTC
Author: thopre01
Date: Wed Dec  7 18:15:52 2016
New Revision: 243375

URL: https://gcc.gnu.org/viewcvs?rev=243375&root=gcc&view=rev
Log:
2016-12-07  Thomas Preud'homme  <thomas.preudhomme@arm.com>

	Backport from mainline
	2016-12-07  Thomas Preud'homme  <thomas.preudhomme@arm.com>

	gcc/
	PR rtl-optimization/78617
	* lra-remat.c (do_remat): Initialize live_hard_regs from live in
	registers, also setting hard registers mapped to pseudo registers.

	gcc/testsuite/
	PR rtl-optimization/78617
	* gcc.c-torture/execute/pr78617.c: New test.


Added:
    branches/ARM/embedded-6-branch/gcc/testsuite/gcc.c-torture/execute/pr78617.c
Modified:
    branches/ARM/embedded-6-branch/gcc/ChangeLog.arm
    branches/ARM/embedded-6-branch/gcc/lra-remat.c
    branches/ARM/embedded-6-branch/gcc/testsuite/ChangeLog.arm
Comment 5 Thomas Preud'homme 2017-01-17 10:10:19 UTC
Author: thopre01
Date: Tue Jan 17 10:09:47 2017
New Revision: 244525

URL: https://gcc.gnu.org/viewcvs?rev=244525&root=gcc&view=rev
Log:
2017-01-17 Thomas Preud'homme <thomas.preudhomme@arm.com>

    Backport from mainline
    2016-12-07 Thomas Preud'homme <thomas.preudhomme@arm.com>

    gcc/
    PR rtl-optimization/78617
    * lra-remat.c (do_remat): Initialize live_hard_regs from live in
    registers, also setting hard registers mapped to pseudo registers.

    gcc/testsuite/
    PR rtl-optimization/78617
    * gcc.c-torture/execute/pr78617.c: New test.

Added:
    branches/gcc-6-branch/gcc/testsuite/gcc.c-torture/execute/pr78617.c
Modified:
    branches/gcc-6-branch/gcc/ChangeLog
    branches/gcc-6-branch/gcc/lra-remat.c
    branches/gcc-6-branch/gcc/testsuite/ChangeLog
Comment 6 Thomas Preud'homme 2017-01-17 10:11:52 UTC
Author: thopre01
Date: Tue Jan 17 10:11:20 2017
New Revision: 244526

URL: https://gcc.gnu.org/viewcvs?rev=244526&root=gcc&view=rev
Log:
2017-01-17 Thomas Preud'homme <thomas.preudhomme@arm.com>

    Backport from mainline
    2016-12-07 Thomas Preud'homme <thomas.preudhomme@arm.com>

    gcc/
    PR rtl-optimization/78617
    * lra-remat.c (do_remat): Initialize live_hard_regs from live in
    registers, also setting hard registers mapped to pseudo registers.

    gcc/testsuite/
    PR rtl-optimization/78617
    * gcc.c-torture/execute/pr78617.c: New test.

Added:
    branches/gcc-5-branch/gcc/testsuite/gcc.c-torture/execute/pr78617.c
Modified:
    branches/gcc-5-branch/gcc/ChangeLog
    branches/gcc-5-branch/gcc/lra-remat.c
    branches/gcc-5-branch/gcc/testsuite/ChangeLog
Comment 7 Thomas Preud'homme 2017-01-17 10:20:02 UTC
Now fixed in all supported branches.
Comment 8 Thomas Preud'homme 2017-01-17 10:24:24 UTC
Adjust Known to work versions and fix Known to fail versions