RFC: make combine do as advertised (cheaper-than)?

[Richard Biener]

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