On Mon, Jul 6, 2020 at 4:03 AM Hans-Peter Nilsson via Gcc-patches
<gcc-patches@gcc.gnu.org> 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.
>
> 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.
>
> Main-interest ports are unsurprisingly pretty tied to this
> effect. I'd love to install the following patch, adjusting the
> function and the two opposing comments. But...it causes
> hundreds of regressions for each of x86_64-linux and
> aarch64-linux (tens for ppc64le-linux), so I'm just not up to
> the task, at least not without previous buy-in from reviewers.
>
> It would need those targets to have their TARGET_INSN_COST
> and/or TARGET_RTX_COSTS functions adjusted.
>
> Alternatives from the top of my head, one of:
>
> - With buy-in from global reviewers, installing this patch on a
> development branch and let all target maintainers adjust their
> target test-cases and cost-functions there, for merge when
> first-class targets are done. (I'm a dreamer.)
>
> - A target combine hook for the decision (passing for inspection
> tuples of from-insns and to-insns and costs) and just falling
> back to the current addition of rtx costs.
>
> - A simpler target combine decision hook that says which one of
> "cheaper" or "as-cheap-as".
A target combine decision hook that on old == new cost
decides between both and given the actual insns? Might
lead to quite hackish target hook implementations...
> - Adjusting documentation and comments that are currently
> untruthful about the cost decision to instead say (to the effect
> of) "as cheap as" instead of "cheaper".
Well, adjusting the function comment to reflect reality is
kind-of obvious ;)
> So, WDYT?
>
> (Tested as above, causing massive pattern-match regressions.)
>
> gcc:
> * combine.c (combine_validate_cost): Reject unless the new total
> cost is cheaper than the original. Adjust the minority of
> comments that don't say "cheaper":
>
> ---
> gcc/combine.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/gcc/combine.c b/gcc/combine.c
> index f69413a..7da144e 100644
> --- a/gcc/combine.c
> +++ b/gcc/combine.c
> @@ -846,8 +846,8 @@ do_SUBST_LINK (struct insn_link **into, struct insn_link
> *newval)
> than the original sequence I0, I1, I2, I3 and undobuf.other_insn. Note
> that I0, I1 and/or NEWI2PAT may be NULL_RTX. Similarly, NEWOTHERPAT and
> undobuf.other_insn may also both be NULL_RTX. Return false if the cost
> - of all the instructions can be estimated and the replacements are more
> - expensive than the original sequence. */
> + of all the instructions can be estimated and the replacements are not
> + cheaper than the original sequence. */
>
> static bool
> combine_validate_cost (rtx_insn *i0, rtx_insn *i1, rtx_insn *i2, rtx_insn
> *i3,
> @@ -938,8 +938,8 @@ combine_validate_cost (rtx_insn *i0, rtx_insn *i1,
> rtx_insn *i2, rtx_insn *i3,
> }
>
> /* Disallow this combination if both new_cost and old_cost are greater than
> - zero, and new_cost is greater than old cost. */
> - int reject = old_cost > 0 && new_cost > old_cost;
> + zero, and new_cost is greater than or equal to the old cost. */
> + int reject = old_cost > 0 && new_cost >= old_cost;
>
> if (dump_file)
> {
> --
> 2.11.0
