Hi everyone,

So I had some difficulty reproducing a regression with my latest optimisation patch, but a couple of days ago I finally reproduced it and isolated the cause.  As explained in the topic, my optimisation was faulty with the instruction pair "movb $-63,%al; movsbw %al,%ax" and other similar constructs - negative constants weren't handled properly.

It seems that not all regressions are caught when you run the test suite conventionally, and you have to specify additional options like "-O4", which may cause other failures to occur if a test is not expecting any kind of intense optimisation, for example.  In response, over here at https://bugs.freepascal.org/view.php?id=36687, I've introduced two new tests that are copies of the ones that failed, but which explicitly add -O4 to the default compiler options.  Besides covering the fault in my patch, I can see these tests proving useful later on because I can see future peephole optimisations in the compiled assembly language (e.g. the following triplet appears... "movw $65280,%ax; cmp $65280,%ax; je @lbl"... logical analysis shows that the conditional branch is always taken).

Long story short, the patch /finally/ works and a full run of the regression suite on i386-win32 and x86_64-win64, including the two new tests, show no new failures.

Gareth aka. Kit

Op 18-02-2020 om 23:04 schreef J. Gareth Moreton:

In fact, *all* test should succeed with an option like -O4. So, following your approach we have to add copies for all tests, with all possible combination of compiler-options. That's undo-able and that is why this isn't done.

So when you are working on optimizations, it might be a good idea to run the complete test-suite with optimizations enabled (-O4) on your local system.

We also realize that it is undo-able to test with all optimization-settings, but to make our live easier, there are nightly (daily) tests with all sort of options. You can find the results here:

https://www.freepascal.org/testsuite/cgi-bin/testsuite.cgi

For example, the latest x86_64-linux run with -O4: https://www.freepascal.org/testsuite/cgi-bin/testsuite.cgi?run1id=488056&run2id=&previousrunid=487938&failedonly=1&action=Show%2FCompare

Regards,

Joost.

Ah, I understand, thank you.

I chose those two tests in particular because the design with multiple typecasts opens it up to a lot of peephole optimisations.

Gareth aka. Kit

J. Gareth Moreton schrieb am Mi., 19. Feb.
2020, 01:40:

The nightly testsuite is run multiple times with different settings, so as Joost said it shouldn't be necessary to single out tests for optimization levels.

Regards,
Sven

Fair enough.  The tests were in a separate patch file anyway so they're easily excluded.  It was more for my benefit anyway because my computer is not the most high-end and frequently runs out of space (one reason why I proposed using SLEB128 and ULEB128 for some of the fields in compiled PPU files, even if partial recompilations took 4 seconds instead of 3 seconds), so I can really only run one configuration overnight (once for the trunk, once for the patches I'm testing under i386-win32, and then again for x86_64-win64.  Linux requires some additional work, but tends to run faster than Windows for me even when using a virtual machine, probably because it's more streamlined with starting new processes).

Gareth aka. Kit

If a particular bug is known to be only triggered by a certain combination of command line switches, the test should be run always with these switches imo.

But then in this specific case the test should be duplicated (as J. Gareth had done in his patch), maybe even moved to a different directory, cause the original test is *not* about the optimization.

Regards,
Sven

True, the test was not about the optimisation, but proved to be the most useful because it turned out that all the typecasting and constant propagation (or lack of) was perfect for seeing how the peephole optimizer handled mov/movzx and mov/movsx pairs.  There was an existing test named tcnvint3b.pp that specifies -Oonoconstprop and then just includes tcnvint3.pp.  For my new tests, tcnvint3a and tcnvint3c, they are copies of those two tests but adding -O4 to the list too.  (3 has no command switches, 3a is -O4, 3b is -Oonoconstprop and 3c is both options).

Gareth

Gareth aka. Kit

J. Gareth Moreton wrote

Long story short, the patch /finally/ works and a full run of the regression suite on i386-win32 and x86_64-win64, including the two new tests, show no new failures.

Great work ! Can you give an overview of vector-code in FPC, both automatic compiler optimizations and explicit user coding ?

Regards,

Adriaan van Os

Yes, but in this case, there is something missed at the node level optimizer. Such code should not be generated.

Long story short, the patch /finally/ works

Which ones did you exactly apply? All three in the bug tracker at once?

The first patch, movzx-choices.patch, is already part of the trunk.  I applied movxx-self-extra.patch amd movx-new-tests.patch (which just contains the tests) to the trunk, which was r44195 at the time, and then also applied just movx-new-tests.patch to a separate branch, then ran regression tests on both branches and compared the results.

Gareth aka. Kit

This patch doesn't address vectorisation issues - currently I don't think the compiler automatically does much code vectorisation, although hopefully that will change down the line.

Gareth aka. Kit

it might be an artefact of "-Oonoconstprop", but if you run all of the "test/cg/tcnvint3" tests with the "-a" option, you will notice such sequences in some of the ".s" file.

Gareth aka. Kit

Then the peephole optimizer should care even less about it.

but if you run all of the "test/cg/tcnvint3" tests with the "-a" option, you will notice such sequences in some of the ".s" file.

With full -O3?

Indeed so, with full -O4 even.  When compiling "/test/cg/tcnvint3.pp" (a test that already exists) with -O4, we get things like this in the assembler dump - command line = "\pp\bin\x86_64-win64\ppcx64 -O4 test\cg\tcnvint3b.pp":

# Peephole Optimization: movq $16711680,%rax -> movl $16711680,%eax (immediate can be represented with just 32 bits)
    movl    $16711680,%eax
    cmpl    $16711680,%eax
    je    .Lj29
    call    P$TCNVINT3_$$_FAIL
    jmp    .Lj30
    .p2align 4,,10
    .p2align 3
.Lj29:

In this case, unless there's a freak CPU error, everything between "je .Lj29" and its destination label will never execute (if "je .Lj29" is changed to "jmp .Lj29", everything between them will be stripped by pass 1 of the peephole optimiser).

Gareth aka. Kit

Oh, sorry, I made a slight error.  The sequences only appear if you specify -Oonoconstprop (and -a).  So that sequence is produced with "\pp\bin\x86_64-win64\ppcx64 -O4 -Oonoconstprop -a test\cg\tcnvint3b.pp"

There are still some inefficient combinations though in the assembly - for example:

    movl    $61441,%eax
    movw    $61441,%ax

Gareth aka. Kit

J. Gareth Moreton wrote

Oh, sorry, I made a slight error.  The sequences only appear if you specify -Oonoconstprop (and -a).  So that sequence is produced with "\pp\bin\x86_64-win64\ppcx64 -O4 -Oonoconstprop -a test\cg\tcnvint3b.pp"

Then this is a non-issue.

This is with full -O3? Did you try to add -Oodeadstore?

Yes, it appeared under -O4.  However, specifying -Oodeadstore caused both instructions to be removed, but it makes sense because a call followed those mov instructions, which sets %rax and, under x86_64-win64, is not taken as a parameter (i.e. the value of %rax is discarded upon calling a subroutine).

Thanks for pointing out where peephole optimisation is wasted and a non-issue.  I need to study nodes more!  *scratches off mov/cmp checks!*

Just to note with the last optimisation over at #36687 that's been giving me hassle until now, it deals mostly with constants that get sign-extended or zero-extended.  For example, in the same test, there are sequences such as this:

    movb    $-63,%al
    movsbl    %al,%eax

... the patch now (correctly) changes that to "movl $-63,%eax". Deadstore and lack of constant propagation isn't affected.

Gareth aka. Kit

Glad it's all fixed now - thanks for the Linux fix, Florian. That blasted "taicpu(p).oper[0]^.ref^.refaddraddr_full" caught me out again!

Gareth aka. Kit

J. Gareth Moreton wrote

Glad it's all fixed now - thanks for the Linux fix, Florian. That blasted "taicpu(p).oper[0]^.ref^.refaddraddr_full" caught me out again!

Yes, the addr_full stuff is a little bit unlucky design ...