Just been looking at the peehole opt (i386). Other than the 2 items already mailed, I found that:

1) Gode as follows is sometimes generated (at various opt levels)

.Ll2:
# [36] i := 1;
movl $1,%eax
.Ll3:
# [38] i := i + 1;
movl $2,%eax

I could not find any code dealing with it, and added some. It does catch a noticeable amount of occurrences during build of fpc and lazarus.

2)
Commented existing code (apparently since revision 1) for {movl [mem1],reg1 to movl [mem1],reg1
movl [mem1],reg2 movl reg1,reg2 }

It used
(taicpu(p).oper[1]^.regtaicpu(hp1).oper[0]^^.ref^.base) and (taicpu(p).oper[1]^.regtaicpu(hp1).oper[0]^^.ref^.index) then but should only compare the supregister part
I replaced that
not(RegInOp(getsupreg(taicpu(p).oper[1]^.reg),taicpu(hp1).oper[0]^)) then

uncommented, and tested.
It does catch a big lot of occurrences.

Both pass all tests form the test dir (except those failing already (see my other mail / compared at slightly diff settings)

Both build a working FPC and Lazarus.

Patch with all 3 changes at the end of the mail. I will separate them, if all else is ok, and needed.

New Code
for removing double move to register. Code is in a block where opsize, and other restraints have already been confirmed.

if (taicpu(p).oper[1]^.typ = top_reg) and
(taicpu(hp1).oper[1]^.typ = top_reg) and
(taicpu(p).oper[1]^.reg =
taicpu(hp1).oper[1]^.reg) and
not(RegInOp(getsupreg(taicpu(p).oper[1]^.reg),taicpu(hp1).oper[0]^)) then { We have mov xxx, reg1 mov yyy, reg1 }

                               begin
                                 asml.remove(p);
                                 p.free;
                                 p := hp1;
                               end

existing, now uncommented
else
{movl [mem1],reg1 to movl [mem1],reg1
movl [mem1],reg2 movl reg1,reg2 }
if (taicpu(p).oper[0]^.typ = top_ref) and
(taicpu(p).oper[1]^.typ = top_reg) and
(taicpu(hp1).oper[0]^.typ = top_ref) and
(taicpu(hp1).oper[1]^.typ = top_reg) and
RefsEqual(TReference(taicpu(p).oper[0]^.ref^),taicpu(hp1).oper[0]^.ref^) and not(RegInOp(getsupreg(taicpu(p).oper[1]^.reg),taicpu(hp1).oper[0]^)) then //(taicpu(p).oper[1]^.regtaicpu(hp1).oper[0]^^.ref^.base) and //(taicpu(p).oper[1]^.regtaicpu(hp1).oper[0]^^.ref^.index) then

                                 begin

taicpu(hp1).loadReg(0,taicpu(p).oper[1]^.reg)

                                 end

Index: compiler/i386/popt386.pas
=================================================================== --- compiler/i386/popt386.pas (revision 26519)
+++ compiler/i386/popt386.pas (working copy)
@@ -1369,39 +1369,50 @@

                                  end
                              end
                            else

-(* {movl [mem1],reg1
- movl [mem1],reg2
- to:
- movl [mem1],reg1
- movl reg1,reg2 }
- if (taicpu(p).oper[0]^.typ = top_ref) and
- (taicpu(p).oper[1]^.typ = top_reg) and
- (taicpu(hp1).oper[0]^.typ = top_ref) and
- (taicpu(hp1).oper[1]^.typ = top_reg) and
- (taicpu(p).opsize = taicpu(hp1).opsize) and
- RefsEqual(TReference(taicpu(p).oper[0]^^),taicpu(hp1).oper[0]^^.ref^) and
- (taicpu(p).oper[1]^.regtaicpu(hp1).oper[0]^^.ref^.base) and
- (taicpu(p).oper[1]^.regtaicpu(hp1).oper[0]^^.ref^.index) then
- taicpu(hp1).loadReg(0,taicpu(p).oper[1]^.reg)
- else*)
- { movl const1,[mem1]
- movl [mem1],reg1
- to:
- movl const1,reg1
- movl reg1,[mem1] }
- if (taicpu(p).oper[0]^.typ = top_const) and
- (taicpu(p).oper[1]^.typ = top_ref) and
+ if (taicpu(p).oper[1]^.typ = top_reg) and
+ (taicpu(hp1).oper[1]^.typ = top_reg) and
+ (taicpu(p).oper[1]^.reg = taicpu(hp1).oper[1]^.reg) and
+ not(RegInOp(getsupreg(taicpu(p).oper[1]^.reg),taicpu(hp1).oper[0]^)) then
+ { We have mov xxx, reg1 mov yyy, reg1 }
+ begin
+ asml.remove(p);
+ p.free;
+ p := hp1;
+ end
+ else
+ {movl [mem1],reg1 to movl [mem1],reg1
+ movl [mem1],reg2 movl reg1,reg2 }
+ if (taicpu(p).oper[0]^.typ = top_ref) and
+ (taicpu(p).oper[1]^.typ = top_reg) and (taicpu(hp1).oper[0]^.typ = top_ref) and
(taicpu(hp1).oper[1]^.typ = top_reg) and
- (taicpu(p).opsize = taicpu(hp1).opsize) and
- RefsEqual(taicpu(hp1).oper[0]^.ref^,taicpu(p).oper[1]^.ref^) and -
not(reginref(getsupreg(taicpu(hp1).oper[1]^.reg),taicpu(hp1).oper[0]^.ref^)) then
+
RefsEqual(TReference(taicpu(p).oper[0]^.ref^),taicpu(hp1).oper[0]^.ref^) and
+ not(RegInOp(getsupreg(taicpu(p).oper[1]^.reg),taicpu(hp1).oper[0]^)) then
+ //(taicpu(p).oper[1]^.regtaicpu(hp1).oper[0]^^.ref^.base) and
+ //(taicpu(p).oper[1]^.regtaicpu(hp1).oper[0]^^.ref^.index) then

                                  begin

- allocregbetween(asml,taicpu(hp1).oper[1]^.reg,p,hp1,usedregs);
- taicpu(hp1).loadReg(0,taicpu(hp1).oper[1]^.reg);
- taicpu(hp1).loadRef(1,taicpu(p).oper[1]^.ref^);
- taicpu(p).loadReg(1,taicpu(hp1).oper[0]^.reg);
+ taicpu(hp1).loadReg(0,taicpu(p).oper[1]^.reg)

                                  end

+ else
+ { movl const1,[mem1]
+ movl [mem1],reg1
+ to:
+ movl const1,reg1
+ movl reg1,[mem1] }
+ if (taicpu(p).oper[0]^.typ = top_const) and
+ (taicpu(p).oper[1]^.typ = top_ref) and
+ (taicpu(hp1).oper[0]^.typ = top_ref) and
+ (taicpu(hp1).oper[1]^.typ = top_reg) and
+ RefsEqual(taicpu(hp1).oper[0]^.ref^,taicpu(p).oper[1]^.ref^) and +
not(reginref(getsupreg(taicpu(hp1).oper[1]^.reg),taicpu(hp1).oper[0]^.ref^)) then
+ begin
+ allocregbetween(asml,taicpu(hp1).oper[1]^.reg,p,hp1,usedregs);
+ taicpu(hp1).loadReg(0,taicpu(hp1).oper[1]^.reg);
+ taicpu(hp1).loadRef(1,taicpu(p).oper[1]^.ref^);
+ taicpu(p).loadReg(1,taicpu(hp1).oper[0]^.reg);
+ // Storing the value to memory should be part of the first instruction
+ taicpu(hp1).fileinfo := taicpu(p).fileinfo;
+ end;

                          end;
                        if GetNextInstruction(p, hp1) and

MatchInstruction(hp1,A_BTS,A_BTR,[Taicpu(p).opsize]) and

There is none I think.

and added some. It does catch a noticeable amount of occurrences during build of fpc and lazarus.

Go ahead. According to me experience a good mean besides testing compiler and lazarus to quickly catch errors in the peephole optimizer is to compare assembler before and after.

The code was commented 15 years ago :) with the comment

* split the optimizer

by Jonas. So I think such statements were caught by the old assembler cse optimizer which is disabled now.

Can you post some example code? It might be worth to think about improving this already in at the node level.

I will try to find some. (I just enabled it, and put a writeln in there, to see, if it was triggered. Then run the tests and buli Lazarus.

In the meantime, what about the other additions/changes?

I already wrote code for them, and mailed it.
So what I need to know: How to best go on to get them accepted and into the compiler?

One problem is, if I create 3 or 4 patches, and report them, only one will work. The others may then conflict. Or I need to create them, based on each other, and only applicable in the right order.

I can do all in one (if that is preferred)...

There are 100 or 1000nds of matches when compiling the IDE, not sure how many are triggered by the below, or by which other condition.

2 examples in below, but they only happen with -O- or -O1 In TFoo.Bar move the comment to the other statement, for 2nd example (if init is called within Init, then different code is produced)

In both cases the 2nd register should not be needed at all. (but if present, is better loaded from

program project1; {$mode objfpc}
type  TFoo = class
     a,b : Integer;
     function Bar: Boolean;
     procedure Init; virtual;
   end;
function TFoo.Bar: Boolean;
begin
   Result := a = b;
//  Init;
end;
procedure TFoo.Init;  begin  end;
begin
end.

//////////// Result := a = b;
.section .text.n_p$project1$_$tfoo_$__$$_bar$$boolean,"x" .balign 16,0x90
.globl P$PROJECT1$_$TFOO_$__$$_BAR$$BOOLEAN
P$PROJECT1$_$TFOO_$__$$_BAR$$BOOLEAN:
# Temps allocated between ebp-8 and ebp-8
# [project1.lpr]
# [14] begin
pushl %ebp
movl %esp,%ebp
leal -8(%esp),%esp
# Var $self located at ebp-4
# Var $result located at ebp-8
movl %eax,-4(%ebp)
# [15] Init;
movl -4(%ebp),%eax // <<<<<<<<<<<<<<<<<<<<<<<<<<<<< movl -4(%ebp),%edx // <<<<<<<<<<<<<<<<<<<<<<<<<<<<< movl (%edx),%edx
call *100(%edx)
# [16] end;
leave
ret

///////////// INIT
.section .text.n_p$project1$_$tfoo_$__$$_bar$$boolean,"x" .balign 16,0x90
.globl P$PROJECT1$_$TFOO_$__$$_BAR$$BOOLEAN
P$PROJECT1$_$TFOO_$__$$_BAR$$BOOLEAN:
# Temps allocated between ebp-8 and ebp-8
# [project1.lpr]
# [14] begin
pushl %ebp
movl %esp,%ebp
leal -8(%esp),%esp
# Var $self located at ebp-4
# Var $result located at ebp-8
movl %eax,-4(%ebp)
# [15] Init;
movl -4(%ebp),%eax
movl -4(%ebp),%edx
movl (%edx),%edx
call *100(%edx)
# [16] end;
leave
ret

Florian Klämpfl wrote

Can you post some example code? It might be worth to think about improving this already in at the node level.

While getting examples, another issue:

with -O2 , -O3 or -O4

Note the
movl %eax,%edx
movl %edx,%eax

with -O1 it is
movl %eax,-4(%ebp)
movl -4(%ebp),%eax

.section .text.n_p$project1$_$tfoo_$__$$_init,"x"
.balign 16,0x90
.globl P$PROJECT1$_$TFOO_$__$$_INIT
P$PROJECT1$_$TFOO_$__$$_INIT:
# Temps allocated between esp+0 and esp+0
# Var $self located in register edx
# [19] begin
movl %eax,%edx
# Var $self located in register edx
# [20] Init;
movl %edx,%eax
movl (%edx),%edx
call *100(%edx)
# [21] end;
ret

program project1;
{$mode objfpc}
type
   { TFoo }
   TFoo = class
     a,b : Integer;
     function Bar: Boolean;
     procedure Init; virtual;
   end;
function TFoo.Bar: Boolean;
begin
   Result := a  b;
end;
procedure TFoo.Init;
begin
   Init;
end;

{ TFoo }

begin
end.

Submit them to a bug report, I can look during the weekend into them.

Well, -O1 can be neglected ...

Ok.

There is another idea, I had in that context, and like to bring up. Though maybe this is already solved in a different way.

One of the optimizations you said it where better avoided to be created in first. I agree.
Only, even if that is archived at some time, who guarantees that it will not be back (and unnoticed)?

Are there tests, that can detect this?
Of course all generated assembler could be parsed back, and analysed. But that is rather a lot of work.

Or the code could be added, and asserted not to be triggered. Of course adding actual test code to the compiler is not a good way either. Unless assert style / Only compiled in if -Sa or -dXXX is given.

Would anything in that direction make sense?
Then the code could work in real live, until no longer needed, at which time it was still useful for assertion.

Just an idea.

Not really, this is something I'am thinking about already for years :(

Of course all generated assembler could be parsed back, and analysed. But that is rather a lot of work.

Yes.

I think this is hard to achive as well.

Then the code could work in real live, until no longer needed, at which time it was still useful for assertion.

Probably the best way would be benchmarking regarding runtime and procedure size: significant changes in runtime or procedure size would trigger an alert.

Why?

I see 2 scenarios. The peephole optimization is either
- always present (even if never triggered.
- contained in $IFDEF BUILD_WITH_TEST_ASSERT

Then there is
   {$IFDEF BUILD_WITH_TEST_ASSERT{
asml.insertbefore(tai_comment.Create(strpnew('BAD_TEST_TRIGGERED')), p);
{$ENDIF}

For the test, the compiler needs to be build with the define. Then it runs with -al ( [1] it can build al testcases, or even the compiler units, or rtl units / and there can be dedicated been written units to be compiled)
The assembler in grepped for BAD_TEST_TRIGGERED, it MUST NOT be found.

[1] The test is not complete, but it is impossible to feed every thinkable input, as that is an infinite amount. However compiling a huge amount of units gives a good chance of catching violations.

I might be overlooking something, but it should not be so hard. For readability those optimization can either be in an include fileso there will be lines like
{$DEFINE TEST_INCLUDE_PART_FOO} {$I TEST_INCLUDE}

Or it can be in a separate function, running an extra loop, at the end or begin of the peephole pass.

I consider it as complicated and it covers only cases one can forsee. Some statistical analysis of benchmark timings and procedure sizes is imo much more general.

Ok, so we were talking about 2 different targets.

You were talking (if I understand correct) about a general test for any and all forms of regressions (with regards to speed or size / not with regards to function) in code generation. This is indeed hard to test. Size may be do able by comparing to a known size that was once archived / size may increase or decrease, but then tests need to be fixed (decrease must be fixed, so that increase from the new optimum will then be detected.)
Speed is indeed very hard. Since even a benchmark may vary.

I was talking about checking for specific/known code snippets that are known to be inefficient (so anything that the peephole generator can/could detect). This is only a small subset of the possible speed/size regressions, but it is at least something.

Yes.

Yes, but having n=5 and doing daily benchmarking should one enable to identify trends.

Yes and no. It is extra code and extra code is always bad ;) and it requires a separate compiler run. I wouldn't waste effort in it.

Florian Klaempfl wrote

Yes and no. It is extra code and extra code is always bad ;) and it requires a separate compiler run. I wouldn't waste effort in it.

testcase are extra code too. ;) scnr

Ok, i see what you mean. No problem. It was just an idea.

Florian Klämpfl wrote

Submit them to a bug report, I can look during the weekend into them.

Done: 0025584, 0025586, 0025587

http://bugs.freepascal.org/view.php?id=25584
http://bugs.freepascal.org/view.php?id=25586
http://bugs.freepascal.org/view.php?id=25587