Re: [PATCH] [testsuite] [powerpc] adjust -m32 counts for fold-vec-extract*

[Segher Boessenkool <segher@kernel.crashing.org>]

Hi Alex,

On Thu, May 25, 2023 at 10:55:37AM -0300, Alexandre Oliva wrote:
> On May 25, 2023, Segher Boessenkool <segher@kernel.crashing.org> wrote:
> > Fwiw, updating the insn counts blindly like this
> 
> ... is a claim that carries a wildly incorrect and insulting underlying
> assumption:

Sorry you feel that way.  I'm not even assuming anything :-(

> I've actually identified the corresponding change to the
> lp64 tests, compared the effects of the codegen changes, and concluded
> the tests needed this changing for ilp32 to keep on testing for the same
> thing after code changes brought about by changes that AFAICT had been
> well understood when making the lp64 adjustments.

But you didn't explain any of that (saying it is so is not the same
thing at all as explaining it!)

> > If it is not possible to keep these tests up-to-date easily
> 
> The counts have been stable for a couple of release cycles already.
> 
> The change that caused the codegen differences is identified and
> understood; the PR confirmed my findings, naming the root cause and the
> incomplete testsuite adjustment.

Oh, was this discussed in some PR?  The patch submission should have
carried the conclusions from the discussions there then :-)

> I suspect there may also be ABI-related assumptions implied by the 'add'
> counts, but I don't know enough about all the ppc variants to be sure.

The compiler can and will create all kinds of code for wildly unexpected
reasons.  "add" is dangerous to count already, but it is not as bad as
"addi" :-)

> Now, if your implied claim is correct that counting 'add/addi'
> instructions in these tests is fragile, dropping the checks for those
> would probably be best.

The same is true for almost all instructions.  You can only sanely count
instructions if either you count only unusual insns, or if you test only
*tiny* functions (say five insns, including the blr at the end!)

> But if ppc maintainers seem to have different
> opinions as to how to deal with the fallout of that one-time codegen
> change, it would be foolish for me to get pulled into the cross fire.

There is no crossfire.  I did not dis-approve the patch, just said this
is a high maintenance direction to proceed in.  There has been a lot of
that the last few years, we should improve on that.  It is not about
this patch (only).

> Here's the patch that corrects the long-broken counts, with the
> requested adjustments, retested with ppc- and ppc64-vx7r2.  Ok?

> Codegen changes caused add instruction count mismatches on
> ppc-*-linux-gnu and other 32-bit ppc targets.  At some point the
> expected counts were adjusted for lp64, but ilp32 differences
> remained, and published test results confirm it.

... and this is not something that can be confimed like this.  Just
spend a few minutes more to put *actual numbers* here, with some
indication this is good and correct codegen, so that it is bloody easy
for a reviewer to review and for a maintainer to approve!

>  /* -m32 target has an 'add' in place of one of the 'addi'. */
> -/* { dg-final { scan-assembler-times {\maddi\M|\madd\M} 2 { target lp64 } } } */
> -/* { dg-final { scan-assembler-times {\maddi\M|\madd\M} 3 { target ilp32 } } } */
> +/* { dg-final { scan-assembler-times {\maddi\M|\madd\M} 2 } } */

Just {\madd} or more conservative {\maddi?\M} then?


Segher