Bug 40956 - Constants are never candidates for hoisting
Summary: Constants are never candidates for hoisting
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: rtl-optimization (show other bugs)
Version: 4.5.0
: P3 enhancement
Target Milestone: ---
Assignee: Maxim Kuvyrkov
URL:
Keywords:
Depends on:
Blocks: 33828
  Show dependency treegraph
 
Reported: 2009-08-03 22:54 UTC by Carrot
Modified: 2010-07-27 21:10 UTC (History)
4 users (show)

See Also:
Host:
Target: arm-eabi
Build:
Known to work:
Known to fail:
Last reconfirmed: 2010-06-19 19:57:16


Attachments
test case (78 bytes, text/plain)
2009-08-03 22:55 UTC, Carrot
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Carrot 2009-08-03 22:54:50 UTC
Compile the following function with options -Os -mthumb -march=armv5te -frename-registers

int foo(int p, int* q)
{
  if (p!=9)
    *q = 0;
  else
    *(q+1) = 0;
  return 3;
}

GCC generates:

        push    {lr}
        cmp     r0, #9         // D
        beq     .L2
        mov     r3, #0         // A
        str     r3, [r1]
        b       .L3
.L2:
        mov     r0, #0         // B
        str     r0, [r1, #4]   // C
.L3:
        mov     r0, #3
        pop     {pc}

If we replace r0 with r3 in instructions B and C, then A and B will be same. So we can move the same instruction before the instruction D and reduce 1 instruction.

Is it a gcse opportunity?
Comment 1 Carrot 2009-08-03 22:55:25 UTC
Created attachment 18294 [details]
test case
Comment 2 Steven Bosscher 2009-08-04 09:05:53 UTC
Hoisting

*** This bug has been marked as a duplicate of 23286 ***
Comment 3 davidxl 2009-12-23 19:37:11 UTC
This bug is ARM specific (thumb) mode. In x86, the hoisting is unnecessary as the move instruction support the imm form. 

The issue here is more in the GIMPLE canonicalization (target specific). In this case, the IR should be in the following form to expose the hoisting.

if (...) {
   temp = 0;
   *p = temp;
}
else
{
   temp = 0;
   *(p+1) = temp;
}
Comment 4 Steven Bosscher 2010-03-19 18:39:39 UTC
Comment #3 makes no sense: There is no such thing as target specific GIMPLE canonicalization. And also there is no hoisting for GIMPLE so the form of the IR given in comment #3 wouldn't make a difference.

Even without -frename-registers, the extra insn is there. GCC trunk revision 157579 generates the following code with "-Os -mthumb -march=armv5te":

foo:
	push	{lr}
	cmp	r0, #9
	beq	.L2
	mov	r3, #0
	str	r3, [r1]
	b	.L3
.L2:
	mov	r3, #0
	str	r3, [r1, #4]
.L3:
	mov	r0, #3
	@ sp needed for prologue
	pop	{pc}

Here the "mov r3, #0" after ".L2:" and after "beq .L2" could be unified e.g. with hoisting or with head-merging (inverse of tail merging).


Just before the RTL hoisting pass, the code looks like this (.152r.cprop1 dump):

    5 NOTE_INSN_BASIC_BLOCK
    2 r135:SI=r0:SI
      REG_DEAD: r0:SI
    3 r136:SI=r1:SI
      REG_DEAD: r1:SI
    4 NOTE_INSN_FUNCTION_BEG
    7 pc={(r135:SI==0x9)?L13:pc}
      REG_DEAD: r135:SI
      REG_BR_PROB: 0xec6
    8 NOTE_INSN_BASIC_BLOCK
    9 r137:SI=0x0
   10 [r136:SI]=r137:SI
      REG_EQUAL: 0x0
      REG_DEAD: r137:SI
      REG_DEAD: r136:SI
L13:
   14 NOTE_INSN_BASIC_BLOCK
   15 r138:SI=0x0
   16 [r136:SI+0x4]=r138:SI
      REG_EQUAL: 0x0
      REG_DEAD: r138:SI
      REG_DEAD: r136:SI
L17:
   18 NOTE_INSN_BASIC_BLOCK
   23 r0:SI=0x3
   26 use r0:SI

Note the insns "9 r137:SI=0x0" and "15 r138:SI=0x0". This could be hoisted in the RTL HOIST pass. But this doesn't happen because the hoisting pass doesn't record constants as hoisting candidates. The "expression passes" in gcse.c (PRE and HOIST) ignore all instructions of the form "(set (reg) (const))" because want_to_gcse_p() returns false for them. This is a deliberate decision in the implementation.

Thus, there is nothing target specific about this problem. The analysis of comment #3 makes no sense, but this is also not really a dup of bug 23286.
Comment 5 Maxim Kuvyrkov 2010-06-19 19:57:16 UTC
I'm working on this bug among other improvements to RTL hoist pass.

My plan is to enable hoisting of such simple constants, but only on very short distances, like 3-5 instructions, tunable through a new parameter.  Targets like x86 can disable simple constant hoisting altogether, while targets like ARM would be able to enable it when optimizing for size.
Comment 6 Maxim Kuvyrkov 2010-07-27 19:35:23 UTC
Subject: Bug 40956

Author: mkuvyrkov
Date: Tue Jul 27 19:34:55 2010
New Revision: 162590

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=162590
Log:
	PR rtl-optimization/40956
	PR target/42495
	PR middle-end/42574
	* gcse.c (compute_code_hoist_vbeinout): Consider more expressions
	for hoisting.
	(hoist_code): Count occurences in current block too.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/gcse.c

Comment 7 Maxim Kuvyrkov 2010-07-27 19:46:44 UTC
Subject: Bug 40956

Author: mkuvyrkov
Date: Tue Jul 27 19:46:26 2010
New Revision: 162596

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=162596
Log:
	PR rtl-optimization/40956
	* config/arm/arm.c (thumb1_size_rtx_costs): Fix cost of simple
	constants.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/arm/arm.c

Comment 8 Maxim Kuvyrkov 2010-07-27 21:06:56 UTC
Subject: Bug 40956

Author: mkuvyrkov
Date: Tue Jul 27 21:06:31 2010
New Revision: 162600

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=162600
Log:
	PR rtl-optimization/40956
	PR target/42495
	PR middle-end/42574
	* gcc.target/arm/pr40956.c, gcc.target/arm/pr42495.c,
	* gcc.target/arm/pr42574.c: Add tests.

Added:
    trunk/gcc/testsuite/gcc.target/arm/pr40956.c
    trunk/gcc/testsuite/gcc.target/arm/pr42495.c
    trunk/gcc/testsuite/gcc.target/arm/pr42574.c
Modified:
    trunk/gcc/testsuite/ChangeLog

Comment 9 Maxim Kuvyrkov 2010-07-27 21:10:54 UTC
Should be fixed now by the above patch series.