Patches item #2044424, was opened at 2008-08-10 01:24
Message generated for change (Comment added) made by maurogiachero
You can respond by visiting:
https://sourceforge.net/tracker/?func=detail&atid=300599&aid=2044424&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: None
Group: None
Status: Open
Resolution: None
Priority: 5
Private: No
Submitted By: Mauro Giachero (maurogiachero)
Assigned to: Nobody/Anonymous (nobody)
Summary: PIC16: vfprintf
Initial Comment:
Hello again.
This time my work towards having no regression failures on PIC16 brought me to vfprintf, the root of the *printf implementation for PIC16.
Regression failures were due to the much partial implementation of that function, so I started working towards a more complete version.
Now, there are way too many *printf* implementations in the sdcc source tree for my limited brain, even looking only at those logically reachable from PIC16:
device/lib/printf_fast.c
device/lib/printf_tiny.c
These are mcs51-specific (shouldn't these go in device/lib/mcs51?).
device/lib/printfl.c
Contains little assembly, and is much limited.
device/lib/printf_large.c
Looks portable and full-featured. Unfortunatly integrating it into
PIC16 looked nontrivial, and most probably would generate very
large code.
device/lib/pic16/libc/stdio/vfprintf.c
Current PIC16 implementation for these functions, with basic
features.
device/lib/pic16/libc/stdio/printf_tiny.c
Looks like a cut-down version of the above.
device/lib/pic16/libc/stdio/printf_small.c
Even more cut-down.
So it looks like there's no full-featured *printf available for PIC16, with the only option of porting printf_large or write a new one.
The attached is an improved version of vfprintf.c. I didn't even try to make a diff since there are simply too many changes.
The first thing one notices is are the 5 switches defining which features should be enabled. These switches actually clutter the code horribly, their main purposes were to evaluate the features impact one by one and to allow a proper comparison with the old code. Maybe merging some of them so that only ~2 switches remain is a good idea. Maybe not.
Without any switch enabled, the code is 7 words smaller than the old version, and there is actually a bug fix (printing 16-bit non-decimal values with high bit set). With all switches on, the code passes the snprintf.c regression test without float support (which is only tested on ds390 and mcs51). The code is 1.6x larger than the old version.
I think PIC16 deserves a reasonable *printf implementation, so merging this code can be a good thing.
My opinion anyway is that long-term there should be:
- a common infrastructure (all ports should have their *printf*
implementations based on the same function [that is, functions
with the same prototype]);
- a basic portable implementation, maybe with switches {dis,en}abling
less-used features of that function;
- port-specific implementations for optimization sake.
My impression instead is that all ports have their own partial implementations and approach the problem in different ways, causing duplicate efforts and much confusion in the head of the poor souls trying to put their hands there ;-).
Just my 2 cents.
That's it. I hope you'll like the code.
With best regards
Mauro
----------------------------------------------------------------------
>Comment By: Mauro Giachero (maurogiachero)
Date: 2008-08-12 21:55
Message:
Logged In: YES
user_id=2160854
Originator: YES
Apparently the discussion here stalled.
I think we both agree that there should be a portable *printf version
that all ports (new and old) can use. I don't think that having
port-specific highly optimized versions is bad per-se, but that's
another story.
But making a unified good version is something that can require quite
a large amount of time. Even more if we want to have 2 or more
versions available due to the possibly large Makefile modifications.
The situation gets even worse if we want to extend the concept to all
other library functions.
Putting this vfprintf version in sdcc, maybe after removing all the
feature selection #defines, is IMHO a good short-term solution to
provide good *printf to PIC16. This implementation is also portable,
so can be used as one of the bases for the unified version.
My suggestion is to apply it. The unification opera can then start
and proceed gradually.
Regards
Mauro
PS: I'll be on vacation/away till about the first week of September,
and I most probably won't reply to mails in that timeframe.
Sorry about that. I'll read mail and reply as soon as I'm back.
----------------------------------------------------------------------
Comment By: Mauro Giachero (maurogiachero)
Date: 2008-08-10 19:11
Message:
Logged In: YES
user_id=2160854
Originator: YES
Hello Maarten,
> Have you tried to compile printf_large for pic16?
Now yes!
To make a quick test I:
- deleted
fprintf.c
printf.c
printf_small.c
printf_tiny.c
sprintf.c
vfprintf.c
vprintf.c
vsprintf.c
from device/lib/pic16/libc/stdio/
- copied
printf_large.c
sprintf.c
vprintf.c
from device/lib/ to device/lib/pic16/libc/stdio/
- copied
stdbool.h
from device/include/ to device/include/pic16
- applied the attached patch (pic16-stdio-printf_large.patch)
and it successfully built and completed the snprintf regression test
(and bug1057979.c, once thrown the PIC16-specific amendments).
Then I enabled float support and the build broke, apparently due to
a compiler fault.
> How does it compare to your version?
Your library has a larger code footprint than the code I submitted,
but uses less RAM.
For comparison:
; Statistics PRINTF_LARGE:
; code size: 2770 (0x0ad2) bytes ( 2.11%)
; 1385 (0x0569) words
; udata size: 20 (0x0014) bytes ( 1.56%)
; access size: 24 (0x0018) bytes
---
; Statistics VFPRINTF:
; code size: 2062 (0x080e) bytes ( 1.57%)
; 1031 (0x0407) words
; udata size: 16 (0x0010) bytes ( 1.25%)
; access size: 31 (0x001f) bytes
I don't know whether the comparison is fair, I didn't look much at the
printf_large code so maybe even without float support it has more
features than the code I submitted. I also haven't played with
SDCC_STACK_AUTO.
> [...] I don't like preprocessor feature-switches in library code
> because they require recompilation. Only for floating point support
> is it appropriate as it requires so much resources.
I almost agree. I think that there's space for three implementations:
- one full-featured;
- one without floating point support;
- one very basic with no modifiers etc, only supporting chars, strings
and signed+unsigned longs in hex (lowercase), oct and dec, and
always returning 0.
Limitations should be well documented, and it should be possible to
switch from one to the other with a command-line switch or a #define
(something like having in stdio.h:
#ifndef _PRINTF
# define _PRINTF 1
#endif
#if _PRINTF == 0
# define printf _printf_bare
#elif _PRINTF == 1
# define printf _printf_nofloat
#else
# define printf _printf_full
#end
with the other *printf as well, and starting sdcc with -D_PRINTF=x).
All versions should be compiled and available by default for all
targets.
Regards
Mauro
File Added: pic16-stdio-printf_large.patch
----------------------------------------------------------------------
Comment By: Maarten Brock (maartenbrock)
Date: 2008-08-10 14:01
Message:
Logged In: YES
user_id=888171
Originator: NO
Mauro,
I fully agree there should be only one common implementation. I do not
believe in hand optimized assembly in the libraries which is why I removed
it from printf_large a long time ago. My goal was to make it fully portable
and as complete and bugfree as possible. I don't like preprocessor
feature-switches in library code because they require recompilation. Only
for floating point support is it appropriate as it requires so much
resources. IMO optimization should come from the compiler itself.
Have you tried to compile printf_large for pic16? How does it compare to
your version?
Greets,
Maarten
----------------------------------------------------------------------
You can respond by visiting:
https://sourceforge.net/tracker/?func=detail&atid=300599&aid=2044424&group_id=599
|