As I just looked through some of the code, I found the following.

compiler\i386\popt386.pas

                 case taicpu(p).opcode Of
....
// line  1122
                   A_MOV:

has several "if" statements, some concatenated with else, but some independent.

if (taicpu(p).oper[1]^.typ = top_reg) and
(getsupreg(taicpu(p).oper[1]^.reg) in [RS_EAX,
RS_EBX, RS_ECX, RS_EDX, RS_ESI, RS_EDI]) and
GetNextInstruction(p, hp1) and
(tai(hp1).typ = ait_instruction) and
....
else
if (taicpu(p).oper[0]^.typ = top_reg) and
(taicpu(p).oper[1]^.typ = top_reg) and
GetNextInstruction(p,hp1) and
(tai(hp1).typ = ait_instruction) and
....
// NO ELSE
if GetNextInstruction(p, hp1) and
....
// NO ELSE
if GetNextInstruction(p, hp1) and
MatchInstruction(hp1,A_BTS,A_BTR,[Taicpu(p).opsize]) and ...
...

In each of them GetNextInstruction(p, hp1) could be executed.

If I counted correct, it can be called up to 6 times for each a_mov. On the other hand:
- no code in this case block, is ever executed, if it does not succeed.
- It will always be executed at least once

So it would probably be better to have this once at the start of the A_MOV block.

If that makes sense, I can check other blocks for similar issues.

I guess the idea is to catch typical code sequences in one go, see also the comment in 1243:
{ Next instruction is also a MOV ? }

It is still on my todo list though to update the peephole optimizer and make a common one for i386 and x86_64 ...

So then a patch would be accepted to move the check for GetNextInstruction(p, hp1)
to the start, and have one single such condition, in one IF block, that will contain all the others?

Martin Frb wrote

So then a patch would be accepted to move the check for GetNextInstruction(p, hp1) to the start, and have one single such condition, in one IF block, that will contain all the others?

The main issue with the peephole optimizer isn't its speed, but its readability, maintainability and extensibility. So unless this significantly improves either (or at the very least doesn't hurt them), I wouldn't modify it.

Jonas

Looking at the below example, I would say it does not hurt, IMHO it is slightly more readable. Give that currently each IF has several lines connected with AND.
Reducing the AND allows to see where they really differ.

But, yes it is at bes a minor readability improvment. Of course readability is dependent on the reader.

As for speed, the differenc in the overall compile time will probably be tiny. Not even sure if noticeable. (I may test it, but maybe not)

I understand, that this is the change, someone with long standing commit rights of its own may do. But it simply is not adding enough value to really spent the time on reviewing it, if it is not your own work. So if it is this then that is ok.

NEW:

If GetNextInstruction(p, hp1) then Begin
   if ffo1 then
     else
       if ffo2  then
       ;
   if ffo3 then
end;

OLD:

   if ffo1 and
       GetNextInstruction(p, hp1) then
     else
       if ffo2   and
           GetNextInstruction(p, hp1) then
       ;
   if ffo3 and
      GetNextInstruction(p, hp1) then

On 1/21/2014 4:23 PM, Martin Frb wrote:
Sorry, but those two code blocks are NOT identical as you claim. At least in the snippets below, GetNextInstruction is called at least twice in the OLD snippet, not only once as your supposed NEW snippet does. It is called once for either the ffo1 or the ffo2 case and again, for the the ffo3 case which is executed after either ffo1 or ffo2.

---
This email is free from viruses and malware because avast! Antivirus protection is active. http://www.avast.com

Florian Klämpfl wrote

It is still on my todo list though to update the peephole optimizer and make a common one for i386 and x86_64 ...

And don't forget i8086 :)

Nikolay

Yes they change the number of invocations.
In the real code, there is at least 1 if block where GetNextInstruction is the first condition. I did write:
- Old/ current code between 1 and 5 invocations
- New code exactly 1
GetNextInstruction is side effect free, it is safe to change the amount of times it is called

You are however right.

If ffo1 or ffo2 changes the next instruction, then it must update the result of GetNextInstruction too (unless it exits the block with "continue" as some, but not all of the blocks do)

This may be a good reason (unless it is known/remembered) to check, why some of the code that applies changes ends with a continue, and other does not (indent or oversight?)

This has at least 2 effects:
1)
- without continue in ffo1/ffo2, it will immediately test for ffo3
- with continue in ffo1/ffo2, it will test for ffo3 in the NEXT loop (since p(the current instruction) is kept.

2)
- with continue the result of the optimization is tested for ALL peephole opts
- without continue it is only tested for ffo3

In case 2, the case "with continue" appears to be preferable. Unless:
- it is known that this would lead to invalid code (but then, that might also happen if the code after round 1 occurs in in-optimized code)
- it is known that no other opt can possible apply (and this is reviewed, each time a new one is added). But then it would do no harm to test anyway.

Might p not be changed by if blocks before?

I think there is little to share that's why I didn't mention it :)

See my other mail, statement can be inserted/deleted, each block that makes modifications, must restore the state. That is it must set hp1 to the correct value again. Of course that could be nil, and that would mean another check on some of the other IF.

But yes, in terms of extending the code later it is a step back: Today all code would be changed to test GetNextInstruction again, if a change was made.
But if someone adds code in future without doing this test, then it breaks.

This is a risk that you/ the fpc team have to decide, if it was acceptable or not.

I could play with the code, and see how much of the IFs would be affected, and provide an actual patch, so it could be compared side by side (which would probably be better, than the current theoretical discussion (even if the patch was discarded it be a good exercise to get to know the code better)

I will defer that though, for
a) got other stuff
b) currently only have one checkout, and other patches pending. That makes it harder to keep track of which change is what.

Well, x86 is x86 is x86. Most of the x86 code generator is already shared in x86/cgx86.pas and that code is common for i8086, i386 and x86_64. IMHO, most of the peephole optimizations can be shared across all x86. Only the 'lea'-related stuff wouldn't work on i8086 (because of the register restrictions of 16-bit references) and there are some optimizations, which are specific to i8086 (like converting certain sequences of movs to lds/les). Of course, I could be wrong, but that was my opinion from just looked to me that most
optimizations done there apply equally well to i8086 as well as x86_64 and it was in my TODO list to eventually cleanup and unify the peephole optimizer across all x86 (or if that turns out to be impractical, at least write one for i8086).

Nikolay

Well, with "some" defines :)

Everything involving addresses must be handled very carefully. A lot is movz/s, shifting or setxx related. So fear overlap is maybe 50%

Which is also on my todo list :)