This is the mail archive of the gcc@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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:

<x-tad-bigger>While I certainly wouldn't expect fold_rtx to find out about this</x-tad-bigger>
<x-tad-bigger>all by itself, I'd have thought that there would have been a</x-tad-bigger>
<x-tad-bigger>REG_EQUIV or REG_EQUAL note that indicates that the end result is</x-tad-bigger>
<x-tad-bigger>the constant (const_double:DF 1.0), and use that in any simplification.</x-tad-bigger>

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: radar-patch-4153339.txt
Description: Text document



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]