Hi!
On Wed, Jun 15, 2022 at 04:19:41PM +0800, Jiufu Guo wrote:
> Segher Boessenkool <seg...@kernel.crashing.org> writes:
> > Have you tried with different limits?
> I drafted cases(C code, and updated asm code) to test runtime costs for
> different code sequences: building constants with 5 insns; with 3
> insns and 2 insns; and loading from rodata.
I'm not asking about trivial examples here. Have you tried the
resulting compiler, on some big real-life code, with various choices for
these essentially random choices, on different cpus?
Huge changes like this need quite a bit of experimental data to back it
up, or some convincing other evidence. That is the only way to move
forward: anything else will resemble Brownian motion :-(
> > And, p10 is a red herring, you
> > actually test if prefixed insns are used.
>
> 'pld' is the instruction which we care about instead target p10 :-). So
> in patch, 'TARGET_PREFIXED' is tested.
Huh. I would think "pli" is the most important thing here! Which is
not a load instruction at all, notwithstanding its name.
> >> --- a/gcc/config/rs6000/rs6000.cc
> >> +++ b/gcc/config/rs6000/rs6000.cc
> >> @@ -9706,8 +9706,9 @@ rs6000_init_stack_protect_guard (void)
> >> static bool
> >> rs6000_cannot_force_const_mem (machine_mode mode ATTRIBUTE_UNUSED, rtx x)
> >> {
> >> - if (GET_CODE (x) == HIGH
> >> - && GET_CODE (XEXP (x, 0)) == UNSPEC)
> >> + /* Exclude CONSTANT HIGH part. e.g.
> >> + (high:DI (symbol_ref:DI ("var") [flags 0xc0] <var_decl>)). */
> >> + if (GET_CODE (x) == HIGH)
> >> return true;
> >
> > So, why is this? You didn't explain.
>
> In the function rs6000_cannot_force_const_mem, we already excluded
> "HIGH with UNSPEC" like: "high:P (unspec:P [symbol_ref..".
> I find, "high:DI (symbol_ref:DI" is similar, which may also need to
> exclude. Half high part of address would be invalid for constant pool.
>
> Or maybe, we could use below code to exclude it.
> /* high part of a symbol_ref could not be put into constant pool. */
> if (GET_CODE (x) == HIGH
> && (GET_CODE (XEXP (x, 0)) == UNSPEC || SYMBOL_REF_P (XEXP (x, 0))))
>
> One interesting thing is how the rtx "(high:DI (symbol_ref:DI (var" is
> generated. It seems in the middle of optimization, it is checked to
> see if ok to store in constant memory.
It probably is fine to not allow the HIGH of *anything* to be put in a
constant pool, sure. But again, that is a change independent of other
things, and it could use some supporting evidence.
> >> case CONST_DOUBLE:
> >> case CONST_WIDE_INT:
> >> + /* It may needs a few insns for const to SET. -1 for outer SET
> >> code. */
> >> + if (outer_code == SET)
> >> + {
> >> + *total = COSTS_N_INSNS (num_insns_constant (x, mode)) - 1;
> >> + return true;
> >> + }
> >> + /* FALLTHRU */
> >> +
> >> case CONST:
> >> case HIGH:
> >> case SYMBOL_REF:
> >
> > But this again isn't an obvious improvement at all, and needs
> > performance testing -- separately of the other changes.
>
> This code updates the cost of constant assigning, we need this to avoid
> CSE to replace the constant loading with constant assigning.
No. This is rtx_cost, which makes a cost estimate of random RTL, not
typically corresponding to existing RTL insns at all. We do not use it
a lot anymore thankfully (we use insn_cost in most places), but things
like CSE still use it.
> The above change (the threshold for storing const in the pool) depends
> on this code.
Yes, and a gazillion other places as well still (or about 50 really :-) )
> > And this is completely independent of the rest as well. Cost 5 or 7 are
> > completely random numbers, why did you pick these? Does it work better
> > than 8, or 4, etc.?
> For 'pld', asm code would be "pld %r9,.LC0@pcrel"; as tests, it is
> faster than 2insns (like, "li+oris"), but still slower than one
> instruction (e.g. li, pli). So, "COSTS_N_INSNS (2) - 1" is selected.
> And with this, cse will not replace 'pld' with 'li + oris'.
(That is WinNT assembler syntax btw. We never use that. We write
either "pld 9,.LC0@pcrel" or "pld r9,.LC0@pcrel".)
COSTS_N_INSNS(1) means 4, always. The costs are not additive at all in
reality of course, so you cannot simply add them and get any sensible
result :-(
Why did you choose 7? Your explanation makes 5 and 6 good candidates
as well. My money is on 6 performing best here, but there is no way to
know without actually trying out things.
> >> --- a/gcc/testsuite/gcc.target/powerpc/medium_offset.c
> >> +++ b/gcc/testsuite/gcc.target/powerpc/medium_offset.c
> >> @@ -1,7 +1,7 @@
> >> /* { dg-do compile { target { powerpc*-*-* } } } */
> >> /* { dg-require-effective-target lp64 } */
> >> /* { dg-options "-O" } */
> >> -/* { dg-final { scan-assembler-not "\\+4611686018427387904" } } */
> >> +/* { dg-final { scan-assembler-times {\msldi|pld\M} 1 } } */
> >
> > Why? This is still better generated in code, no? It should never be
> > loaded from a constant pool (it is hex 4000_0000_0000_0000, easy to
> > construct with just one or two insns).
>
> For p8/9, two insns "lis 0x4000+sldi 32" are used:
> addis %r3,%r2,.LANCHOR0@toc@ha
> addi %r3,%r3,.LANCHOR0@toc@l
> lis %r9,0x4000
> sldi %r9,%r9,32
> add %r3,%r3,%r9
> blr
That does not mean putting this constant in the constant pool is a good
idea at all, of course.
> On p10, as expected, 'pld' would be better than 'lis+sldi'.
Is it?
> And for this case, it also saves other instructions(addis/addi):
> pld %r3,.LC1@pcrel
> blr
This explanation then belongs in your commit message :-)
> > Btw, you need to write
> > \m(?:rldimi|ld|pld)\M
> Oh, thanks! I did not aware of this.
(?:xxx) is just the same as (xxx), but the parentheses in the latter are
capturing, which messes up scan-assembler-times, so you need
non-capturing grouping.
> Maybe I should separate out two cases one for "-mdejagnu-cpu=power10",
> and one for "-mdejagnu-cpu=power7", because the load instructions
> the number would be different.
Yeah, it is probably best to never allow both a load and non-load insns
in the same check.
> > So no doubt this will improve things, but we need testing of each part
> > separately. Also look at code size, or differences in the generated
> > code in general: this is much more sensitive to detect than performance,
> > and is not itself sensitive to things like system load, so a) is easier
> > to measure, and b) has more useful outputs, outputs that tell more of
> > the whole story.
> Thanks for your suggestions!
> I also feel it is very sensitive when I test performance.
Yeah, there are constants *everywhere* in typical compiler output, so a
small change will easily move the needle already.
> The change
> (e.g. cost of constant) may affect other optimization passes indirectly.
That too, yes.
> I will also check the object changes (object size or differences). I may
> use objects from GCC bootstrap and GCC test suite.
Good choice :-)
Looking forward to it,
Segher
