[3/4] Set costs for jumps in combine
diff mbox series

Message ID b9727b99-475a-ce95-6394-a9ece98313c0@t-online.de
State New
Headers show
Series
  • Eliminate cc0 from m68k
Related show

Commit Message

Bernd Schmidt Nov. 13, 2019, 1:13 p.m. UTC
The combiner is somewhat strange about how it uses costs. If any of the
insns involved in a comparison have a cost of 0, it does not verify that
the substitution is cheaper. Also, it does not compute costs for jump
insns, so they are always set to zero. As a consequence, any possible
substitution is performed if a combination into a jump is possible,
which turns out isn't really desirable on m68k with cbranch patterns.

This patch simply removes a test for NONJUMP_INSN_P. Bootstrapped and
tested on the gcc135 machine (powerpc64le-unknown-linux-gnu).


Bernd

Comments

Segher Boessenkool Nov. 13, 2019, 4:16 p.m. UTC | #1
Hi!

On Wed, Nov 13, 2019 at 02:13:48PM +0100, Bernd Schmidt wrote:
> The combiner is somewhat strange about how it uses costs. If any of the
> insns involved in a comparison have a cost of 0, it does not verify that
> the substitution is cheaper.

"Cost 0" means "unknown cost".  This isn't just combine, it is a property
of insn_cost (and insn_rtx_cost before it).  It does make it impossible
for combine to deal with actual zero cost things.

> Also, it does not compute costs for jump
> insns, so they are always set to zero. As a consequence, any possible
> substitution is performed if a combination into a jump is possible,
> which turns out isn't really desirable on m68k with cbranch patterns.
> 
> This patch simply removes a test for NONJUMP_INSN_P. Bootstrapped and
> tested on the gcc135 machine (powerpc64le-unknown-linux-gnu).

I wonder why that test was there.  It was added in r84513, which is where
insn_rtx_cost was made from combine_insn_cost, which didn't have that
non-jump thing yet.

It is still stage 1, so we'll find out if any target breaks I guess.
Okay for trunk.  Thanks!


Segher
Bernd Schmidt Nov. 21, 2019, 1:36 p.m. UTC | #2
On 11/13/19 5:16 PM, Segher Boessenkool wrote:
> On Wed, Nov 13, 2019 at 02:13:48PM +0100, Bernd Schmidt wrote:
>> Also, it does not compute costs for jump
>> insns, so they are always set to zero. As a consequence, any possible
>> substitution is performed if a combination into a jump is possible,
>> which turns out isn't really desirable on m68k with cbranch patterns.
>>
>> This patch simply removes a test for NONJUMP_INSN_P. Bootstrapped and
>> tested on the gcc135 machine (powerpc64le-unknown-linux-gnu).
> 
> I wonder why that test was there.  It was added in r84513, which is where
> insn_rtx_cost was made from combine_insn_cost, which didn't have that
> non-jump thing yet.
> 
> It is still stage 1, so we'll find out if any target breaks I guess.
> Okay for trunk.  Thanks!

Thanks. Just FYI, this is held up a little. I decided I'd also test on
x86, and there it shows a case where ix86_rtx_cost misses something: the
i386/pr30315.c testcase wants to combine compares into addition+jump on
carry, but the rtx_costs show too high a cost for (compare (plus)). I'm
testing a fix for that in i386.c.


Bernd
Segher Boessenkool Nov. 22, 2019, 12:42 a.m. UTC | #3
On Thu, Nov 21, 2019 at 02:36:53PM +0100, Bernd Schmidt wrote:
> On 11/13/19 5:16 PM, Segher Boessenkool wrote:
> > On Wed, Nov 13, 2019 at 02:13:48PM +0100, Bernd Schmidt wrote:
> >> Also, it does not compute costs for jump
> >> insns, so they are always set to zero. As a consequence, any possible
> >> substitution is performed if a combination into a jump is possible,
> >> which turns out isn't really desirable on m68k with cbranch patterns.
> >>
> >> This patch simply removes a test for NONJUMP_INSN_P. Bootstrapped and
> >> tested on the gcc135 machine (powerpc64le-unknown-linux-gnu).
> > 
> > I wonder why that test was there.  It was added in r84513, which is where
> > insn_rtx_cost was made from combine_insn_cost, which didn't have that
> > non-jump thing yet.
> > 
> > It is still stage 1, so we'll find out if any target breaks I guess.
> > Okay for trunk.  Thanks!
> 
> Thanks. Just FYI, this is held up a little. I decided I'd also test on
> x86, and there it shows a case where ix86_rtx_cost misses something: the
> i386/pr30315.c testcase wants to combine compares into addition+jump on
> carry, but the rtx_costs show too high a cost for (compare (plus)). I'm
> testing a fix for that in i386.c.

Maybe i386 should implement the insn_cost hook as well?  For most targets
that is a lot simpler to get right than rtx_cost, but allowing memory in
many insns and all the different insn lengths complicates matters.  At
least insn_cost isn't inside-out, that should make it easier to deal with
already.


Segher
Bernd Schmidt Nov. 22, 2019, 1:05 a.m. UTC | #4
On 11/22/19 1:42 AM, Segher Boessenkool wrote:
> On Thu, Nov 21, 2019 at 02:36:53PM +0100, Bernd Schmidt wrote:
>> Thanks. Just FYI, this is held up a little. I decided I'd also test on
>> x86, and there it shows a case where ix86_rtx_cost misses something: the
>> i386/pr30315.c testcase wants to combine compares into addition+jump on
>> carry, but the rtx_costs show too high a cost for (compare (plus)). I'm
>> testing a fix for that in i386.c.
> 
> Maybe i386 should implement the insn_cost hook as well?  For most targets
> that is a lot simpler to get right than rtx_cost, but allowing memory in
> many insns and all the different insn lengths complicates matters.  At
> least insn_cost isn't inside-out, that should make it easier to deal with
> already.

That kind of thing is up to the x86 maintainers. I think the problem at
hand can be fixed quite simply by detecting PLUS inside COMPARE and just
counting it like we would a normal PLUS. Patch will follow once testing
is complete.


Bernd
Paul Koning Nov. 22, 2019, 1:12 a.m. UTC | #5
> On Nov 21, 2019, at 7:42 PM, Segher Boessenkool <segher@kernel.crashing.org> wrote:
> 
> ...
> Maybe i386 should implement the insn_cost hook as well?  For most targets
> that is a lot simpler to get right than rtx_cost, but allowing memory in
> many insns and all the different insn lengths complicates matters.  At
> least insn_cost isn't inside-out, that should make it easier to deal with
> already.

Yes, rtx_cost is a pain to write and understand.  I looked at insn_cost in the past but was told, or got the impression somehow, that it is at the moment a partial solution and rtx_cost is still needed to complete the cost picture.

Is that incorrect, or no longer accurate?  I would like to dump rtx_cost in my target if that is now indeed a valid thing to do.

	paul
Segher Boessenkool Nov. 22, 2019, 4:47 p.m. UTC | #6
On Thu, Nov 21, 2019 at 08:12:05PM -0500, Paul Koning wrote:
> > On Nov 21, 2019, at 7:42 PM, Segher Boessenkool <segher@kernel.crashing.org> wrote:
> > 
> > ...
> > Maybe i386 should implement the insn_cost hook as well?  For most targets
> > that is a lot simpler to get right than rtx_cost, but allowing memory in
> > many insns and all the different insn lengths complicates matters.  At
> > least insn_cost isn't inside-out, that should make it easier to deal with
> > already.
> 
> Yes, rtx_cost is a pain to write and understand.  I looked at insn_cost in the past but was told, or got the impression somehow, that it is at the moment a partial solution and rtx_cost is still needed to complete the cost picture.
> 
> Is that incorrect, or no longer accurate?  I would like to dump rtx_cost in my target if that is now indeed a valid thing to do.

Some things still want the rtx_cost of non-insns.  CSE does this, as does
expand (for figuring out the multiplication strategy to use, for one thing),
and then there are set_src_cost and set_rtx_cost (which are probably not
hard to fix, for the cases where there *is* an insn for doing things).

With rtx_cost you need to estimate the cost of arbitrary RTL expressions,
whether that means anything for the machine or not.  This does not make
terribly much sense, but changing the compiler her without regressing
things isn't trivial.


Segher

Patch
diff mbox series

	* combine.c (combine_instructions): Record costs for jumps.

diff --git a/gcc/combine.c b/gcc/combine.c
index 857ea30dafd..9446d2769ab 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -1234,8 +1234,7 @@  combine_instructions (rtx_insn *f, unsigned int nregs)
 						    insn);
 
 	    /* Record the current insn_cost of this instruction.  */
-	    if (NONJUMP_INSN_P (insn))
-	      INSN_COST (insn) = insn_cost (insn, optimize_this_for_speed_p);
+	    INSN_COST (insn) = insn_cost (insn, optimize_this_for_speed_p);
 	    if (dump_file)
 	      {
 		fprintf (dump_file, "insn_cost %d for ", INSN_COST (insn));