Richard Biener <rguent...@suse.de> writes: > On Fri, 25 Feb 2022, Jiufu Guo wrote: > >> Richard Biener <rguent...@suse.de> writes: >> >> > On Thu, 24 Feb 2022, Jiufu Guo wrote: >> > >> >> Jiufu Guo via Gcc-patches <gcc-patches@gcc.gnu.org> writes: >> >> >> >> > Segher Boessenkool <seg...@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: