Hi!

Richard Biener <rguent...@suse.de> writes:

> On Thu, 10 Mar 2022, Jiufu Guo wrote:
>
>> 
>> Hi!
>> 
>> Richard Biener <rguent...@suse.de> writes:
>> 
>> > On Wed, 9 Mar 2022, Jiufu Guo wrote:
>> >
>> >> 
>> >> Hi!
>> >> 
>> >> Richard Biener <rguent...@suse.de> writes:
>> >> 
>> >> > On Tue, 8 Mar 2022, Jiufu Guo wrote:
>> >> >
>> >> >> Jiufu Guo <guoji...@linux.ibm.com> writes:
>> >> >> 
>> >> >> Hi!
>> >> >> 
>> >> >> > Hi Sehger,
>> >> >> >
>> >> >> > Segher Boessenkool <seg...@kernel.crashing.org> writes:
>> >> >> >
>> >> >> >> 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.
>> >> >> > Get it!
>> >> >> >>
>> >> >> >>> 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.
>> >> >> 
>> >> >> I drafted a new patch that replace rtx_cost with insn_cost for cse.cc.
>> >> >> Different from the previous partial patch, this patch replaces all 
>> >> >> usage
>> >> >> of rtx_cost. It may be better/aggressive than previous one.
>> >> >
>> >> > I think there's no advantage for using insn_cost over rtx_cost for
>> >> > the simple SET case.
>> >> 
>> >> Thanks for your comments and raise this concern.
>> >> 
>> >> For those targets which do not implement insn_cost, insn_cost calls
>> >> rtx_cost through pattern_cost, then insn_cost is equal to rtx_cost.
>> >> 
>> >> While, for those targets which have insn_cost, it seems insn_cost would
>> >> be better(or say more accurate/consistent?) than rtx_cost. Since:
>> >> - insn_cost recog the insn first, and compute cost through something
>> >
>> > target hooks are expected to call recog on the insn but the generic
>> > fallback does not!?  Or do you say a target _could_ call recog?
>> > I think it would be valid to only expect recognized insns here
>> > and thus cse.cc obligation would be to call regoc on the "fake"
>> > instruction which then raises the obvious issue whether you should
>> > ever call recog on something "fake".
>> Thanks Richard! I also feel this is a main point of insn_cost.
>> From my understanding: it would be better to let insn_cost check
>> the valid recognizable insns; this would be the major purpose of
>> insn_cost.  While, I'm also wondering, we may let it go for 'fake'
>> instruction for current implementations (e.g. arm_insn_cost) which
>> behaviors to walk/check pattern. 
>
> Note for the CSE case you'll always end up with the single move
> pattern so it's somewhat pointless to do the recog & insn_cost
> dance there.  For move cost using rtx_cost (SET, ..) should
> be good enough.  One could argue we should standardize a (wrapping)
> API like move_cost (enum machine_mode mode, rtx to, rtx from),
> with to/from optional (if omitted a REG of MODE).  But the existing
> rtx_cost target hook implementation should be sufficient to handle
> it, without building a fake insn and without doing (the pointless,
> if not failing) recog process.

Thanks so much for your comments and suggestions!

Using rtx_cost is able to handle those things in cse.cc.
For insn_cost, it is good for checking an instruction.  To simulate
'rtx_cost' using insn_cost, there is excess work: 'recog' on the setting
of rtx expr and alternative estimation. 

I just wondering we may prefer to use insn_cost for its consistency and
accuracy. :-) So, the patch is prepared.


BR,
Jiufu

>
> Richard.
>
>> >
>> > I also see that rs6000_insn_cost does
>> >
>> > static int
>> > rs6000_insn_cost (rtx_insn *insn, bool speed)
>> > {
>> >   if (recog_memoized (insn) < 0)
>> >     return 0;
>> >
>> > so not recognized insns become quite cheap - it looks like
>> > insn_cost has no documented failure mode and initial implementors
>> > chickened out asserting that an insn is / can be recognized.
>> > In fact I'd assert that INSN_CODE (insn) >= 0 simply because there's
>> > no failure mode and the _caller_ should have made sure the
>> > insn can be recognized.  That said, the insn_cost should do that.
>> Thanks! Using the assert would help to catch un-recog failures.
>> 
>> > (is INSN_CODE () == 0 a real thing?)
>> 0 is specialy, it is some thing like CODE_FOR_nothing. :-)
>> 
>> >
>> >> (like length/cost attributes from .md file) for the 'machine insn'.
>> >> - rtx_cost estimates the cost through analyzing the 'rtx content'.
>> >> The accurate estimation relates to the context.
>> >> 
>> >> For a special example: "%r100 = C", as a previous patch, by tunning
>> >> target's rtx_cost hook, cost could be computed according to the value
>> >> of C. insn_cost may just model the cost in the define of the machine
>> >> instruction.
>> >> 
>> >> These reasons are my initial thoughts.  Segher may have better
>> >> explain. :-) 
>> >
>> > OK, so in theory the targets insn_cost can use insn attributes
>> > which allows to keep the cost parts of an instruction close to the
>> > instruction patterns which is probably a good thing for
>> > maintainability.  But as you point out this requires recognizing
>> > of possibly generated instructions.  And before reload it has
>> > the issue that the alternative is not determined which means the
>> > true cost cannot be determined without walking the pattern,
>> > like on x86 where many instruction operands have both register
>> > and memory alternatives.
>> Right, when there is no alternative fits for true cost, it may have to
>> check the pattern like 'rtx_cost' then. :-)
>> 
>> 
>> BR,
>> Jiufu
>> 
>> >
>> >> To replace rtx_cost with insn_cost, this patch build a SET instruction:
>> >> "%r = rtx_expr", then using "%r = rtx_expr" from insn_cost to simulate
>> >> the cost of "rtx_expr" from rtx_cost.
>> >> 
>> >> 
>> >> BR,
>> >> Jiufu
>> >> 
>> >> >
>> >> > Richard.
>> >> >
>> >> >> With this patch, bootstrap pass.
>> >> >> From regtest, only output of fusion-p10-ldcmpi.c is changed, and the
>> >> >> change seems as expected.
>> >> >> 
>> >> >> 
>> >> >> BR,
>> >> >> Jiufu
>> >> >> 
>> >> >> diff --git a/gcc/cse.cc b/gcc/cse.cc
>> >> >> index a18b599d324..e623ad298db 100644
>> >> >> --- a/gcc/cse.cc
>> >> >> +++ b/gcc/cse.cc
>> >> >> @@ -262,6 +262,9 @@ static struct qty_table_elem *qty_table;
>> >> >>  static rtx_insn *this_insn;
>> >> >>  static bool optimize_this_for_speed_p;
>> >> >>  
>> >> >> +/* Used for insn_cost. */
>> >> >> +static rtx_insn *estimate_insn;
>> >> >> +
>> >> >>  /* Index by register number, gives the number of the next (or
>> >> >>     previous) register in the chain of registers sharing the same
>> >> >>     value.
>> >> >> @@ -445,7 +448,7 @@ struct table_elt
>> >> >>  /* Compute cost of X, as stored in the `cost' field of a table_elt.  
>> >> >> Fixed
>> >> >>     hard registers and pointers into the frame are the cheapest with a 
>> >> >> cost
>> >> >>     of 0.  Next come pseudos with a cost of one and other hard 
>> >> >> registers with
>> >> >> -   a cost of 2.  Aside from these special cases, call `rtx_cost'.  */
>> >> >> +   a cost of 2.  Aside from these special cases, call `insn_cost'.  */
>> >> >>  
>> >> >>  #define CHEAP_REGNO(N)                                                
>> >> >>         \
>> >> >>    (REGNO_PTR_FRAME_P (N)                                              
>> >> >> \
>> >> >> @@ -698,18 +701,33 @@ preferable (int cost_a, int regcost_a, int 
>> >> >> cost_b, int regcost_b)
>> >> >>     from COST macro to keep it simple.  */
>> >> >>  
>> >> >>  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*/)
>> >> >>  {
>> >> >>    scalar_int_mode int_mode, inner_mode;
>> >> >> -  return ((GET_CODE (x) == SUBREG
>> >> >> -         && REG_P (SUBREG_REG (x))
>> >> >> -         && is_int_mode (mode, &int_mode)
>> >> >> -         && is_int_mode (GET_MODE (SUBREG_REG (x)), &inner_mode)
>> >> >> -         && GET_MODE_SIZE (int_mode) < GET_MODE_SIZE (inner_mode)
>> >> >> -         && subreg_lowpart_p (x)
>> >> >> -         && TRULY_NOOP_TRUNCATION_MODES_P (int_mode, inner_mode))
>> >> >> -        ? 0
>> >> >> -        : rtx_cost (x, mode, outer, opno, optimize_this_for_speed_p) 
>> >> >> * 2);
>> >> >> +  if (GET_CODE (x) == SUBREG && REG_P (SUBREG_REG (x))
>> >> >> +      && is_int_mode (mode, &int_mode)
>> >> >> +      && is_int_mode (GET_MODE (SUBREG_REG (x)), &inner_mode)
>> >> >> +      && GET_MODE_SIZE (int_mode) < GET_MODE_SIZE (inner_mode)
>> >> >> +      && subreg_lowpart_p (x)
>> >> >> +      && TRULY_NOOP_TRUNCATION_MODES_P (int_mode, inner_mode))
>> >> >> +    return 0;
>> >> >> +
>> >> >> +  if (estimate_insn == NULL)
>> >> >> +    {
>> >> >> +      estimate_insn = make_insn_raw (
>> >> >> +      gen_rtx_SET (gen_rtx_REG (mode, LAST_VIRTUAL_REGISTER + 1), x));
>> >> >> +      SET_PREV_INSN (estimate_insn) = NULL_RTX;
>> >> >> +      SET_NEXT_INSN (estimate_insn) = NULL_RTX;
>> >> >> +      INSN_LOCATION (estimate_insn) = 0;
>> >> >> +    }
>> >> >> +  else
>> >> >> +    {
>> >> >> +      /* Update for new context.  */
>> >> >> +      INSN_CODE (estimate_insn) = -1;
>> >> >> +      PUT_MODE (SET_DEST (PATTERN (estimate_insn)), mode);
>> >> >> +      SET_SRC (PATTERN (estimate_insn)) = x;
>> >> >> +    }
>> >> >> +  return insn_cost (estimate_insn, optimize_this_for_speed_p);
>> >> >>  }
>> >> >>  
>> >> >>  
>> >> >> @@ -6667,6 +6685,7 @@ cse_main (rtx_insn *f ATTRIBUTE_UNUSED, int 
>> >> >> nregs)
>> >> >>  
>> >> >>    init_recog ();
>> >> >>    init_alias_analysis ();
>> >> >> +  estimate_insn = NULL;
>> >> >>  
>> >> >>    reg_eqv_table = XNEWVEC (struct reg_eqv_elem, nregs);
>> >> >>  
>> >> >> 
>> >> 
>> 

Reply via email to