From: SourceForge.net <no...@so...> - 2008-09-15 10:12:56
|
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 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 |