[sdcc-devel] [ sdcc-Patches-2044424 ] PIC16: vfprintf

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

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-09-15 18:21

Message:
Hello Borut,

stdio.h should remain untouched, the patch was only related to the
printf_large experiment.

Mauro

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

Comment By: Borut Raem (borutr)
Date: 2008-09-15 18:03

Message:
Hi Mauro,

I red the thread, I understand you general idea, I totally agree that
printf (and also many other library sources) should be unified among all
targets and I also agree that the solution you proposed is a "workaround"
until somebody (you) implements the unified solution. So I'm favorable to
commit your version of vfprintf.

But you still haven't answered my question about stdio.h: should it remain
unchanged or you meant to apply the pic16-stdio-printf_large.patch? But the
patch is probably only for printf_large. So you are right: I read the
thread too superficially ;-)

Borut

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

Comment By: Mauro Giachero (maurogiachero)
Date: 2008-09-15 12:12

Message:
Ok, I should have put a better comment to the patch (but you didn't read
the thread, did you? ;-)).

The proposed vfprintf.c is a drop-in replacement for the original one, no
patch is needed.
The patch comes from my discussion with Maarten about using the existing
printf_large with PIC16. See my 2008-08-10 message.
So, my original proposal was to replace vfprintf.c with no additional
patches.
Adding the BINARY_SPECIFIER switch is OK at least to see whether there is
someone complaining, since %b has a different meaning for all other ports
(maybe this should become %B?).

However, I'be been also thinking about the "printf unification" among all
the ports, and more generally about using generic library implementations
instead of port-specific ones where possible. I'm not sure this thread is
the right place to discuss it (I _need_ to discuss about this before
starting to code), but my current idea is to work on it from November (I
have to finish my Ph.D thesis before that). So this is expected to be an
"interim" solution to a larger problem.

If you apply this new implementation, please remove the PIC16-specific
amendments from bug1057979.c (sorry for not having provided a patch).

Regards
Mauro

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

Comment By: Borut Raem (borutr)
Date: 2008-09-14 19:16

Message:
Hi Mauro,

I took a look to your patch:
vfprintf.c seems OK to me. I added an additional switch to enable %b
specifiers: BINARY_SPECIFIER, which is not defined by default.

I have some problems with stdio.h:
- why did you uncomment the following lines? Where is the definition of
function _print_format?
  //typedef void (*pfn_outputchar)(char c, void* p) _REENTRANT;
  //extern int _print_format (pfn_outputchar pfn, void* pvoid, const char
*format, va_list ap);

- why you removed the following declaration?
  extern unsigned int vfprintf(FILE *, char *, va_list);

- you removed the following declarations:
  void printf_small(char *, ...);
  void printf_tiny(char *, ...);  // __reentrant;
  probably because you meant to substitute them with vfprintf using
different switches?
  I propose to leave them for the time being.

Borut

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

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