[sdcc-devel] [ sdcc-Bugs-1677178 ] Some peephole rules ignored

[SourceForge.net <no...@so...>]

Bugs item #1677178, was opened at 2007-03-09 13:23
Message generated for change (Comment added) made by spth
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=100599&aid=1677178&group_id=599

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: peephole optimizer
Group: None
Status: Open
Resolution: None
Priority: 5
Private: No
Submitted By: Philipp Krause (spth)
Assigned to: Nobody/Anonymous (nobody)
Summary: Some peephole rules ignored

Initial Comment:
It seems some peephole rules are ignored.
I use sdcc 2.6.4 #4600 with my patch #1677140 applied.

I compile the attached test.t using sdcc -mz80 -c test.c

Without peephole optimization I get:

;test.c:23: if(first_moveable == i)
;	genCmpEq
;	AOP_STK for 
; genCmpEq: left 1, right 1, result 0
	ld	a,4(ix)
	ld	iy,#_first_moveable
	cp	0(iy)
	jp	NZ,00108$
	jp	00109$
00108$:
	jp	00103$
00109$:

With peephole optimization I get:

;test.c:23: if(first_moveable == i)
;	genCmpEq
;	AOP_STK for 
; genCmpEq: left 1, right 1, result 0
	ld	a,4(ix)
	ld	iy,#_first_moveable
	cp	0(iy)
	jr	Z,00109$
	jp	00103$
00109$:

It seems the peephole optimizer applies the rule
replace restart {
	jp	NZ,%1
	jp	%2
%1:
} by {
	jp	Z,%2
%1:
} if labelRefCountChange(%1 -1)

and
replace {
	jp	Z,%5
} by {
	jr	Z,%5
} if labelInRange()
which is one of the last rules. But in my peeph.def there is
replace restart {
	jp	Z,%1
	jp	%2
%1:
} by {
	jp	NZ,%2
%1:
} if labelRefCountChange(%1 -1)
as well, which I think should be applied, but it is not.

Philipp


----------------------------------------------------------------------

>Comment By: Philipp Krause (spth)
Date: 2007-03-25 15:26

Message:
Logged In: YES 
user_id=564030
Originator: YES

I've modified the peephole optimizer. It fixes this problem and thus
reduces sice of generated binaries and improves their speed. I got a 2%
size reduction in the real-world program I tested. However sdcc takes more
time to compile programs: +25% for the regression tests, +0% for a library
I use, +25% for the real-world program.

----------------------------------------------------------------------

Comment By: Philipp Krause (spth)
Date: 2007-03-25 13:29

Message:
Logged In: YES 
user_id=564030
Originator: YES

I found the problem: The peephole optimizer will not immediately restart
after having applied a "replace restart" rule. It will instead set a
rrestart flag and restart after ahving tried to apply all subsequent rules.
This can be seen in the for loop at line 2186 in SDCCpeeph.c. This means
that the jp->jr replace rule even though it is one of the last ones will be
applied before matching rules at the start of the peeph.def file if it only
matches due to a change by a peephole rule.
Making the peephole optimizer restart immediately after applying a
"replace restart" rule would fix the problem, making sdcc generate better
code, but make sdcc slower. I'll try to measure the difference it makes.

Philipp


----------------------------------------------------------------------

Comment By: Philipp Krause (spth)
Date: 2007-03-09 16:00

Message:
Logged In: YES 
user_id=564030
Originator: YES

I've found out more about the problem: I changed nearly all restart to
replace restart, got exaclty the same resulting code, then changed
everything to restart and still got the same result.

It seems that replace restart is broken and treted as a simple replace.

I've been adding comment lines to some peepholes and added them in
peepholes I introduced myself, but there are still some old unmodidfed Z80
peepholes around. I can make a patch for that.
Philipp


----------------------------------------------------------------------

Comment By: Frieder Ferlemann (frief)
Date: 2007-03-09 15:47

Message:
Logged In: YES 
user_id=589052
Originator: NO

could you add a comment lines like:

";      peephole 123.f  removed redundant move"

into the "by{...}" sections at the place where
the peephole would make its first change to the
assembly code? 
The mcs51 port does it like this ("Peephole" written 
upper case though - I'm tempted to change that).
Especially as the list of peephole rules grows 
these comments really help debugging:)

In the long term comments like 
"// applies to return 0.0; in f.e. sincosf.c"
might (theoretically) help to find/remove/adapt peepholes
which do not apply anymore due to changes in code generation.


----------------------------------------------------------------------

Comment By: Philipp Krause (spth)
Date: 2007-03-09 14:16

Message:
Logged In: YES 
user_id=564030
Originator: YES

I don't have personal peepholes. I only use the peeph.def in the sdcc
repository as modified by my patch #1677140.
The rule to replace jp by jr is one of the last ones. The other two rules
are in the middle of peeph.def. My patch makes them both "replace restart"
rules.

Philipp


----------------------------------------------------------------------

Comment By: Maarten Brock (maartenbrock)
Date: 2007-03-09 13:55

Message:
Logged In: YES 
user_id=888171
Originator: NO

Hi Philipp,

I think personal peepholes are parsed after the default ones. Are you sure
it didn't replace the jp by a jr already and thus cannot match your rule?
What do you get with only the default peephole optimizations?

Maarten

----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=100599&aid=1677178&group_id=599