On Tue, Mar 01, 2022 at 10:28:57PM +0800, Jiufu Guo wrote:
> Segher Boessenkool <seg...@kernel.crashing.org> writes:
> > No. insn_cost is only for correct, existing instructions, not for
> > made-up nonsense. I created insn_cost precisely to get away from that
> > aspect of rtx_cost (and some other issues, like, it is incredibly hard
> > and cumbersome to write a correct rtx_cost).
>
> Thanks! The implementations of hook insn_cost are align with this
> design, they are checking insn's attributes and COSTS_N_INSNS.
>
> One question on the speciall case:
> For instruction: "r119:DI=0x100803004101001"
> Would we treat it as valid instruction?
Currently we do, alternative 6 in *movdi_internal64: we allow any r<-n.
This is costed as 5 insns (cost=20).
It generally is better to split things into patterns close to the
eventual machine isntructions as early as possible: all the more generic
optimisations can take advantage of that then.
> A patch, which is attached the end of this mail, accepts
> "r119:DI=0x100803004101001" as input of insn_cost.
> In this patch,
> - A tmp instruction is generated via make_insn_raw.
> - A few calls to rtx_cost (in cse_insn) is replaced by insn_cost.
> - In hook of insn_cost, checking the special 'constant' instruction.
> Are these make sense?
I'll review that patch inline.
> > That is one reason why it is better to generate (close to) machine
> > insns as early as possible: it makes it much easier to estimate
> > realistic costs. (Another important reason is it allows other
> > optimisations, without us having to do any work for it!)
> Get it! In the middle of an optimization pass, 'interim'
> instruction maybe acceptable. While it would better to outputs
> only contains 'valid machine insn' from any RTL passes.
Acceptable only if there is a very good reason for it, really :-(
> --- a/gcc/config/rs6000/rs6000.cc
> +++ b/gcc/config/rs6000/rs6000.cc
> @@ -22131,6 +22131,16 @@ rs6000_debug_rtx_costs (rtx x, machine_mode mode,
> int outer_code,
> static int
> rs6000_insn_cost (rtx_insn *insn, bool speed)
> {
> + /* Handle special 'constant int' insn. */
> + rtx set = PATTERN (insn);
> + if (GET_CODE (set) == SET && CONSTANT_P (SET_SRC (set)))
> + {
> + rtx src = SET_SRC (set);
> + machine_mode mode = GET_MODE (SET_DEST (set));
> + if (CONST_INT_P (src) || CONST_WIDE_INT_P (src))
> + return COSTS_N_INSNS (num_insns_constant (src, mode));
> + }
> +
> if (recog_memoized (insn) < 0)
> return 0;
Why would such a set not recog()?
Needs a comment in any case, to say what this is a workaround for.
> +static int insn_cost_x (rtx_insn *, rtx);
Don't declare functions, just put their definitions before their first
use. (And use a better name please :-) )
> static int
> -notreg_cost (rtx x, machine_mode mode, enum rtx_code outer, int opno)
> +notreg_cost (rtx x, machine_mode mode, enum rtx_code outer, int opno,
> + rtx_insn *insn = NULL)
Don't use default arguments like this, it is an abomination.
> @@ -709,9 +713,21 @@ notreg_cost (rtx x, machine_mode mode, enum rtx_code
> outer, int opno)
> && subreg_lowpart_p (x)
> && TRULY_NOOP_TRUNCATION_MODES_P (int_mode, inner_mode))
> ? 0
> + : insn != NULL ? insn_cost_x (insn, x)
> : rtx_cost (x, mode, outer, opno, optimize_this_for_speed_p) * 2);
> }
You can just always use insn_cost? insn_cost -> pattern_cost ->
set_src_cost -> rtx_cost. That works for COST at least, not sure about
COST_IN, maybe that needs a little more care (cse.c works with invalid
insns all over the place :-( )
>
> +/* Internal function, to get cost when use X to replace source of insn
> + which is a SET. */
> +
> +static int
> +insn_cost_x (rtx_insn *insn, rtx x)
> +{
> + INSN_CODE (insn) = -1;
> + SET_SRC (PATTERN (insn)) = x;
> + return insn_cost (insn, optimize_this_for_speed_p);
> +}
You need to restore stuff as well?
> @@ -4603,6 +4619,7 @@ cse_insn (rtx_insn *insn)
>
> Nothing in this loop changes the hash table or the register chains. */
>
> + rtx_insn *tmp_insn = NULL;
> for (i = 0; i < n_sets; i++)
> {
> bool repeat = false;
> @@ -4638,6 +4655,10 @@ cse_insn (rtx_insn *insn)
> mode = GET_MODE (src) == VOIDmode ? GET_MODE (dest) : GET_MODE (src);
> sets[i].mode = mode;
>
> + if (tmp_insn == NULL_RTX && src && dest && dest != pc_rtx
> + && src != pc_rtx)
> + tmp_insn = make_insn_raw (gen_rtx_SET (copy_rtx (dest), copy_rtx(src)));
src and dest are always non-nil here. I'll have to read the code better
to know about the (pc) stuff.
> @@ -5103,7 +5124,7 @@ cse_insn (rtx_insn *insn)
> src_cost = src_regcost = -1;
> else
> {
> - src_cost = COST (src, mode);
> + src_cost = COST_SRC (tmp_insn, src, mode);
I think you can just leave this as COST?
Segher
