Fwd: [RFC] - Regression exposed by recent change to compress_float_constant

Fwd: [RFC] - Regression exposed by recent change to compress_float_constant

[Dale Johannesen]

[-- Attachment #1.1: Type: text/plain, Size: 2697 bytes --]

Fariborz is still having problems with his mailer and has asked me to 
forward this.

On Aug 10, 2005, at 2:35 PM, Dale Johannesen wrote:

>
> On Aug 10, 2005, at 12:43 PM, Fariborz Jahanian wrote:
>
>
>> Following patch has exposed an optimization shortcoming:
>>
>> 2005-07-12 Dale Johannesen <dalej@apple.com>
>>
>>   * expr.c (compress_float_constant): Add cost check.
>>   * config/rs6000.c (rs6000_rtx_cost): Adjust FLOAT_EXTEND cost.
>>
>> This patch results in generating worse code for the following test 
>> case:
>>
>> 1) Test case:
>>
>> struct S {
>>   float d1, d2, d3;
>>
>
> I believe you mean double not float; the RTL snippets you give 
> indicate this.
>
>
>> (insn 12 7 13 0 (set (reg:SF 59)
>>   (mem/u/i:SF (symbol_ref/u:SI ("*LC0") [flags 0x2]) [0 S4 A32])) -1 
>> (nil)
>>   (nil))
>>
>> (insn 13 12 14 0 (set (mem/s/j:DF (reg/f:SI 58 [ D.1929 ]) [0 
>> <result>.d1+0 S8 A32])
>>   (float_extend:DF (reg:SF 59))) -1 (nil)
>>   (nil))
>>
>
> However, if you try your example with float as given, you see it does 
> not do a
> direct store of constant 0 with or without the compress_float patch. 
> IMO the
> compress_float patch does not really have anything to do with this 
> problem;
> before this patch the double case was working well by accident, my 
> patch
> exposed a problem farther downstream, which was always there for the 
> float
> case.
>
> When I put that patch in, rth remarked:
>
> While I certainly wouldn't expect fold_rtx to find out about this
> all by itself, I'd have thought that there would have been a
> REG_EQUIV or REG_EQUAL note that indicates that the end result is
> the constant (const_double:DF 1.0), and use that in any simplification.
>
> Indeed there is no such note, and I suspect adding it somewhere 
> (expand?) would fix this.
It turned out that cse does put REG_EQUIV on the insn which sets load 
of "LC0" to the register. So, no need to do this. It also tells me that 
cse is expected to use this information to do the constant propagation 
(which in the example test case is the next few insns). Attached patch 
accomplishes this task. It is against apple local branch. It has been 
bootstrapped and dejagnu tested on x86-darwin, ppc-darwin. Note that 
patch is similar to the code right before it (which is also shown in 
this patch), so there is a precedence for this type of a fix. If this 
looks reasonable, I will prepare an FSF patch.

ChangeLog:

2005-08-19  Fariborz Jahanian <fjahanian@apple.com>

        * cse.c (cse_insn): Use the constant to propagte
        into the rhs of a set insn which is a register.
        This is cheaper.




[-- Attachment #1.2: Type: text/enriched, Size: 3097 bytes --]

Fariborz is still having problems with his mailer and has asked me to
forward this.


On Aug 10, 2005, at 2:35 PM, Dale Johannesen wrote:


<excerpt>

On Aug 10, 2005, at 12:43 PM, Fariborz Jahanian wrote:



<excerpt>Following patch has exposed an optimization shortcoming:


2005-07-12 Dale Johannesen
<<<color><param>0000,0000,EEEE</param>dalej@apple.com</color>>


  * expr.c (compress_float_constant): Add cost check.

  * config/rs6000.c (rs6000_rtx_cost): Adjust FLOAT_EXTEND cost.


This patch results in generating worse code for the following test
case:


1) Test case:


struct S {

  float d1, d2, d3;


</excerpt>

I believe you mean double not float; the RTL snippets you give
indicate this.



<excerpt>(insn 12 7 13 0 (set (reg:SF 59)

  (mem/u/i:SF (symbol_ref/u:SI ("*LC0") [flags 0x2]) [0 S4 A32])) -1
(nil)

  (nil))


(insn 13 12 14 0 (set (mem/s/j:DF (reg/f:SI 58 [ D.1929 ]) [0
<<result>.d1+0 S8 A32])

  (float_extend:DF (reg:SF 59))) -1 (nil)

  (nil))


</excerpt>

However, if you try your example with float as given, you see it does
not do a 

direct store of constant 0 with or without the compress_float patch.
IMO the

compress_float patch does not really have anything to do with this
problem;

before this patch the double case was working well by accident, my
patch

exposed a problem farther downstream, which was always there for the
float

case.


When I put that patch in, rth remarked:


<fontfamily><param>Courier</param><x-tad-bigger>While I certainly
wouldn't expect fold_rtx to find out about this</x-tad-bigger></fontfamily>

<fontfamily><param>Courier</param><x-tad-bigger>all by itself, I'd
have thought that there would have been a</x-tad-bigger></fontfamily>

<fontfamily><param>Courier</param><x-tad-bigger>REG_EQUIV or REG_EQUAL
note that indicates that the end result is</x-tad-bigger></fontfamily>

<fontfamily><param>Courier</param><x-tad-bigger>the constant
(const_double:DF 1.0), and use that in any simplification.</x-tad-bigger></fontfamily>


Indeed there is no such note, and I suspect adding it somewhere
(expand?) would fix this.

</excerpt>It turned out that cse does put REG_EQUIV on the insn which
sets load of "LC0" to the register. So, no need to do this. It also
tells me that cse is expected to use this information to do the
constant propagation (which in the example test case is the next few
insns). Attached patch accomplishes this task. It is against apple
local branch. It has been bootstrapped and dejagnu tested on
x86-darwin, ppc-darwin. Note that patch is similar to the code right
before it (which is also shown in this patch), so there is a
precedence for this type of a fix. If this looks reasonable, I will
prepare an FSF patch.


ChangeLog:


2005-08-19  Fariborz Jahanian
<<<color><param>0000,0000,EEEE</param>fjahanian@apple.com</color>>


        * cse.c (cse_insn): Use the constant to propagte

        into the rhs of a set insn which is a register.

        This is cheaper.





[-- Attachment #2: radar-patch-4153339.txt --]
[-- Type: text/plain, Size: 2292 bytes --]

Index: cse.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/cse.c,v
retrieving revision 1.342.4.3
diff -c -p -r1.342.4.3 cse.c
*** cse.c	5 Jul 2005 23:21:50 -0000	1.342.4.3
--- cse.c	19 Aug 2005 18:21:56 -0000
*************** cse_insn (rtx insn, rtx libcall_insn)
*** 5455,5460 ****
--- 5455,5469 ----
        if (dest == pc_rtx && src_const && GET_CODE (src_const) == LABEL_REF)
  	src_folded = src_const, src_folded_cost = src_folded_regcost = -1;
  
+       /* APPLE LOCAL begin radar 4153339 */
+       if (n_sets == 1 && GET_CODE (sets[i].src) == REG 
+ 	  && src_const && GET_CODE (src_const) == CONST_DOUBLE)
+ 	{
+ 	  src_folded = src_const;
+ 	  src_folded_cost = src_folded_regcost = -1;
+ 	}
+       /* APPLE LOCAL end radar 4153339 */
+ 
        /* Terminate loop when replacement made.  This must terminate since
           the current contents will be tested and will always be valid.  */
        while (1)
Index: testsuite/ChangeLog.apple-ppc
===================================================================
RCS file: /cvs/gcc/gcc/gcc/testsuite/Attic/ChangeLog.apple-ppc,v
retrieving revision 1.1.4.88
diff -c -p -r1.1.4.88 ChangeLog.apple-ppc
*** testsuite/ChangeLog.apple-ppc	15 Aug 2005 21:02:26 -0000	1.1.4.88
--- testsuite/ChangeLog.apple-ppc	19 Aug 2005 18:21:59 -0000
***************
*** 1,3 ****
--- 1,8 ----
+ 2005-08-18  Fariborz Jahanian <fjahanian@apple.com>
+ 
+ 	Radar 4153339
+ 	* gcc.dg/i386-movl-float.c: New.
+ 
  2005-08-15  Devang Patel  <dpatel@apple.com>
  
  	Radar 4209318
Index: testsuite/gcc.dg/i386-movl-float.c
===================================================================
RCS file: testsuite/gcc.dg/i386-movl-float.c
diff -N testsuite/gcc.dg/i386-movl-float.c
*** /dev/null	1 Jan 1970 00:00:00 -0000
--- testsuite/gcc.dg/i386-movl-float.c	19 Aug 2005 18:22:03 -0000
***************
*** 0 ****
--- 1,15 ----
+ /* APPLE LOCAL begin radar 4153339 */
+ /* { dg-do compile { target i?86-*-* x86_64-*-* } } */
+ /* { dg-options "-O1 -mdynamic-no-pic -march=pentium4 -mtune=prescott" } */
+ /* { dg-final { scan-assembler-times "movl\[^\\n\]*" 8} } */
+ 
+ struct S {
+ 	double d1, d2, d3;
+ };
+ 
+ struct S ms()
+ {
+ 	struct S s = {0,0,0};
+ 	return s;
+ }
+ /* APPLE LOCAL end radar 4153339 */

[-- Attachment #3.1: Type: text/plain, Size: 1 bytes --]



[-- Attachment #3.2: Type: text/enriched, Size: 2 bytes --]

Re: Fwd: [RFC] - Regression exposed by recent change to compress_float_constant

[Ian Lance Taylor]

On Aug 10, 2005, at 12:43 PM, Fariborz Jahanian wrote:

> +       /* APPLE LOCAL begin radar 4153339 */
> +       if (n_sets == 1 && GET_CODE (sets[i].src) == REG 
> + 	  && src_const && GET_CODE (src_const) == CONST_DOUBLE)
> + 	{
> + 	  src_folded = src_const;
> + 	  src_folded_cost = src_folded_regcost = -1;
> + 	}
> +       /* APPLE LOCAL end radar 4153339 */

I don't see how this could be right for FSF gcc.  It's putting a
target specific decision into the target independent code.  It is
simply not true that a constant double is always cheaper than a
register, for example.  The similar case you cite, of the indirect
jump insn, really is target independent--a branch to a known
destination is always going to be cheaper than an indirect branch via
a register.

It seems possible that this could be fixed by adjusting the cost of
the CONST_DOUBLE in the target backend.

Ian