| Message ID | b9727b99-475a-ce95-6394-a9ece98313c0@t-online.de |
|---|---|
| State | New |
| Headers | show |
| Series | Eliminate cc0 from m68k | expand |
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
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
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
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
> 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
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
* 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));