(Back from vacation, found that this had an unanswered question, quoted last.)
> From: Segher Boessenkool <seg...@kernel.crashing.org> > Date: Tue, 14 Jul 2020 23:58:05 +0200 > Hi! > > On Mon, Jul 06, 2020 at 04:01:54AM +0200, Hans-Peter Nilsson via Gcc-patches > wrote: > > Most comments, including the second sentence in the head comment > > of combine_validate_cost, the main decision-maker of the combine > > pass, refer to the function as returning true if the new > > insns(s) *cheaper* than the old insns, when in fact the function > > returned true also if the cost was the same. Returning true for > > cheaper also seems more sane than as-cheap-as considering the > > need to avoid oscillation between same-cost combinations. Also, > > it makes the job of later passes harder, having combine make > > more complex combinations of the same cost. > > All of this was added in https://gcc.gnu.org/g:64b8935d4809 , which also > adds the > > + /* Disallow this recombination if both new_cost and old_cost are > + greater than zero, and new_cost is greater than old cost. */ > > comment (which is what it actually does). Before that change, combine > didn't look at costs at all. > > Combine never has used a "strictly smaller than" cost thing. (I suggest we use terms of generally greater/lower cost, instead of smaller/greater cost, to avoid confusion whether "smaller" refers to the general cost or just code size. JFTR: yes, I know which one is considered depends on use of -Os and equivalents - currently. :) Right, we both looked at the history of combine.c. The idea of *cost* "cheaper than" (I read it as "lower than") was introduced with the comments and documentation misleadingly saying "are cheaper than", and it was in that same commit. There's at least two of them, not counting various comments mentioning various sub-costs: +/* Subroutine of try_combine. Determine whether the combine replacement + patterns NEWPAT and NEWI2PAT are cheaper according to combine_insn_cost + that the original instruction sequence I1, I2 and I3. Note that I1 + /* Only allow this combination if combine_insn_costs reports that the + replacement instructions are cheaper than the originals. */ > > Right, you can affect this with your target TARGET_RTX_COSTS and > > TARGET_INSN_COST hooks, but only for trivial cases, and you have > > increasingly more complex combinations (many-to-many > > combinations) where you have to twist simple logic to appease > > combine (stop it from combining) or give up. > > There are 2-1, 3-1, 4-1, 3-2, 4-2, which all reduce the number of insns, > and then there is 2-2, which gets rid of one log_link. If the isnn_cost > stays the same, it always wins something else. That "something else" is presumptious. A longer dependency-chain may sum up to faster execution considering parallel or OoO execution. Someone adding another N-M combination will notice, after two more years of work. :) Perhaps a log_links dependency can be turned into INSN_COST-based metric, by using a target hook defaulting to 1? > > Alternatives from the top of my head, one of: > > ... > > 5) Improve your target so that its insn_cost reflects ithe costs of > the insns better. I see cheap gnawing, I hope I didn't add any of that myself. I already covered target costs before the 1..4 list and you actually quoted that (the quoted paragraph above your log_links comment). > Can you share some typical examples where things are worse with the > current behaviour? To recap, it all began with the observation of comments in combine.c saying "cheaper than", with the code implementing "same or cheaper", together with the flaw fixed by 84c5396d4bdbf which I belive is a sufficient conclusion for that specific instance. A lower cost also seems more consistent with other decisions. (BTW, the commit is a bit misleading; I believe all "case #2" cc0 convertees will benefit from an accurate is_just_move, not just CRIS.) The fixed 2-2 case is all the "typical examples" I had so far. Remaining suggestions are just fixing perceived inconsistencies. brgds, H-P
