Re: [PATCH] Check if loading const from mem is faster

[Jiufu Guo <guojiufu@linux.ibm.com>]

Richard Biener <rguenther@suse.de> writes:

> On Fri, 25 Feb 2022, Jiufu Guo wrote:
>
>> Richard Biener <rguenther@suse.de> writes:
>> 
>> > On Thu, 24 Feb 2022, Jiufu Guo wrote:
>> >
>> >> Jiufu Guo via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
>> >> 
>> >> > Segher Boessenkool <segher@kernel.crashing.org> writes:
>> >> >
>> >> >> On Wed, Feb 23, 2022 at 02:02:59PM +0100, Richard Biener wrote:
>> >> >>> I'm assuming we're always dealing with
>> >> >>> 
>> >> >>>   (set (reg:MODE ..) <src_folded>)
>> >> >>> 
>> >> >>> here and CSE is not substituting into random places of an
>> >> >>> instruction(?).  I don't know what 'rtx_cost' should evaluate
>> >> >>> to for a constant, if it should implicitely evaluate the cost
>> >> >>> of putting the result into a register for example.
>> >> >>
>> >> >> rtx_cost is no good here (and in most places).  rtx_cost should be 0
>> >> >> for anything that is used as input in a machine instruction -- but you
>> >> >> need much more context to determine that.  insn_cost is much simpler and
>> >> >> much easier to use.
>> >> >>
>> >> >>> Using RTX_COST with SET and 1 at least looks no worse than using
>> >> >>> your proposed new target hook and comparing it with the original
>> >> >>> unfolded src (again with SET and 1).
>> >> >>
>> >> >> It is required to generate valid instructions no matter what, before
>> >> >> the pass has finished that is.  On all more modern architectures it is
>> >> >> futile to think you can usefully consider the cost of an RTL expression
>> >> >> and derive a real-world cost of the generated code from that.
>> >> >
>> >> > Thanks Segher for pointing out these!  Here is  another reason that I
>> >> > did not use rtx_cost: in a few passes, there are codes to check the
>> >> > constants and store them in constant pool.  I'm thinking to integerate
>> >> > those codes in a consistent way.
>> >> 
>> >> Hi Segher, Richard!
>> >> 
>> >> I'm thinking the way like: For a constant,
>> >> 1. if the constant could be used as an immediate for the
>> >> instruction, then retreated as an operand;
>> >> 2. otherwise if the constant can not be stored into a
>> >> constant pool, then handle through instructions;
>> >> 3. if it is faster to access constant from pool, then emit
>> >> constant as data(.rodata);
>> >> 4. otherwise, handle the constant by instructions.
>> >> 
>> >> And to store the constant into a pool, besides force_const_mem,
>> >> create reference (toc) may be needed on some platforms.
>> >> 
>> >> For this particular issue in CSE, there is already code that
>> >> tries to put constant into a pool (invoke force_const_mem).
>> >> While the code is too late.  So, we may check the constant
>> >> earlier and store it into constant pool if profitable.
>> >> 
>> >> And another thing as Segher pointed out, CSE is doing too
>> >> much work.  It may be ok to separate the constant handling
>> >> logic from CSE.
>> >
>> > Not sure - CSE just is value numbering, I don't see that it does
>> > more than that.  Yes, it might have developed "heuristics" over
>> > the years what to CSE and to what and where to substitute and
>> > where not.  But in the end it does just value numbering.
>> >
>> >> 
>> >> I update a new version patch as follow (did not seprate CSE):
>> >
>> > How is the new target hook better in any way compared to rtx_cost
>> > or insn_cost?  It looks like a total hack.
>> >
>> > I suppose the actual way of materializing a constant is done
>> > behind GCCs back and not exposed anywhere?  But instead you
>> > claim the constants are valid when they actually are not?
>> > Isn't the problem then that the rs6000 backend lies?
>> 
>> Hi Richard,
>> 
>> Thanks for your comments and sugguestions!
>> 
>> Materializing a constant should be done behind GCC.
>> On rs6000, in expand pass, during emit_move, the constant is
>> checked and store into constant pool if necessary.
>> Some other platforms are doing a similar thing, e.g.
>> ix86_expand_vector_move, alpha_expand_mov,...
>> mips_legitimize_const_move.
>> 
>> But, it does not as we expect, force_const_mem is also 
>> exposed other places (not only ira/reload for stack reference).
>> 
>> CSE is one place, for example, CSE first retrieve the constant
>> from insn's equal, but it also tries to put constant into
>> pool for some condition (the condition was introduced at
>> early age of cse.c, and it is rare to run into in trunk).
>> In some aspects, IMHO, this seems not a great work of CSE.
>> 
>> And this is how the 'invalid(or say slow)' constant occurs.
>> e.g.  before cse:
>>     7: r119:DI=[unspec[`*.LC0',%r2:DI] 47]
>>       REG_EQUAL 0x100803004101001
>> after cse: 
>>     7: r119:DI=0x100803004101001
>>       REG_EQUAL 0x100803004101001
>> 
>> As you pointed out: we can also avoid this transformation through
>> rtx_cost/insn_cost by estimating the COST more accurately for
>>  "r119:DI=0x100803004101001". (r119:DI=0x100803004101001 will not
>> be a real final instruction.)
>
> At which point does this become the final instruction?  I suppose
> CSE belives this constant is legitimate and the insn is recognized
> just fine in CSE?  (that's what I mean with "behind GCCs back")
>
It would become final instructions during split pass on rs6000,
other target, e.g. alpha, seem also do split it.
>> Is it necessary to refine this constant handling for CSE, or even
>> to eliminate the logic on constant extracting for an insn, beside
>> updating rtx_cost/insn_cost?
>
> So if CSE really just transforms
>
>>     7: r119:DI=[unspec[`*.LC0',%r2:DI] 47]
>>       REG_EQUAL 0x100803004101001
>
> to
>
>>     7: r119:DI=0x100803004101001
>>       REG_EQUAL 0x100803004101001
>
> then why can't rtx_cost (SET, 1) or insn_cost () be used to
> accurately tell it that the immediate is going to be a lot
> more expensive?
>
> That is, rtx_cost (CONST_INT-rtx, DI, SET, 1, ...) is accurate
> enough to be treated as an actual insn (it _might_ be part of
> a parallel I guess, but that's unlikely) and thus the backend
> should have a very good idea of the many instruction it needs
> for this.  Likewise rtx_cost ([unspec[`*.LC0',%r2:DI] 47], DI, SET, 1, 
> ...)
> should receive accurate cost that can be compared to other
> rtx_cost (..., DI, SET, 1, ...)
>
> And CSE doesn't even need to create fake insns here since IMHO
> rtx_cost is good enough to capture the full insn.  Targets can
> choose to split out a set_cost from their rtx_cost/insn_cost
> hooks for this case for example.

Hi Richard,

Right, we can fix this issue by updating rtx_cost/insn_cost to
tell that this kind of constants is a lot of expansive.

To update rtx_cost, we can use a trivial patch (shown as end of
this mail) to fix this particular issue.
To use insn_cost, additional work is replacing rtx_cost with
insn_cost for CSE, maybe more suitable for stage1.

So, it would be too far to fix this by refactoring the logic of
constant handling. :-)

Thanks for your comments!

BR,
Jiufu

>
> Richard.
>
>> Any sugguestions?
>> 
>> >
>> > Btw, all of this is of course not appropriate for stage4 and changes
>> > to CSE need testing on more than one target.
>> I would do more evaluation, thanks!
>> 
>> Jiufu
>> 
>> >
>> > Richard.
>> >
>> >> Thanks for the comments and suggestions again!
>> >> 
>> >> 
>> >> BR,
>> >> Jiufu
>> >> 

Part of the experimental patch for rs6000, which could help
to mitigate inaccurate rtx cost on constant.

diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
index e82a47f4c0e..e429ae2bcf0 100644
--- a/gcc/config/rs6000/rs6000.cc
+++ b/gcc/config/rs6000/rs6000.cc
@@ -21884,6 +21884,14 @@ rs6000_rtx_costs (rtx x, machine_mode mode, int outer_code,
 
     case CONST_DOUBLE:
     case CONST_WIDE_INT:
+      /* Set const to a reg, it may needs a few insns.  */
+      if (outer_code == SET)
+	{
+	  *total = COSTS_N_INSNS (num_insns_constant (x, mode));
+	  return true;
+	}
+      /* FALLTHRU */
+
     case CONST:
     case HIGH:
     case SYMBOL_REF: