llvm dev - Feb 2011 - [LLVMdev] Implementing platform specific library call simplification

On Feb 13, 2011, at 12:24 AM, Chris Lattner wrote:
> On Feb 2, 2011, at 10:11 AM, Richard Osborne wrote:
>> The newlib C library provides iprintf(), a restricted version of printf
>> without support for floating-point formatting. I'd like to add an 
>> optimization which turns calls to printf() into calls to iprintf() if 
>> the format string has no floating point specifiers.
> 
> Cool, ok.  I can see how this would be very useful for a variety of
embedded systems.
> 
>> At the moment I've got this working locally by adding code to the 
>> simplify-libcalls pass. However this will break on targets where 
>> iprintf() isn't available. Is there a sensible place to add
information
>> about which library functions are available for a particular target?
> 
> I'd suggest adding a bool argument (HasIPrintf) to the
createSimplifyLibCallsPass function and the pass constructor.  Then clang (or
whatever is setting up the pass manager) can query the appropriate target info
to pass down this flag.
> 
> Longer term, I'd like to introduce a simple interface (via
TargetRegistry) for exposing target libcall info.  This would be useful to
eliminate the HAVE_FLOORF etc #defines in simplifylibcalls, which break cross
compilation.
Hi Richard,

Just to close the loop on this, I went ahead and added a new
llvm/Target/TargetLibraryInfo.h and added it to simplifylibcalls. 
TargetLibraryInfo doesn't handle all the libcalls that SimplifyLibCalls
handles yet, but it should provide a starting point for iprintf: just add an
iprintf enum to TargetLibraryInfo and make your transformation predicated on
TLI->has(LibFunc::iprintf).

It would be great to see this go into mainline,

-Chris

On 2/24/2011 1:21 AM, Chris Lattner wrote:
> On Feb 13, 2011, at 12:24 AM, Chris Lattner wrote:
>
>> On Feb 2, 2011, at 10:11 AM, Richard Osborne wrote:
>>> The newlib C library provides iprintf(), a restricted version of
printf
>>> without support for floating-point formatting. I'd like to add
an
>>> optimization which turns calls to printf() into calls to iprintf()
if
>>> the format string has no floating point specifiers.
>> Cool, ok.  I can see how this would be very useful for a variety of
embedded systems.
>>
>>> At the moment I've got this working locally by adding code to
the
>>> simplify-libcalls pass. However this will break on targets where
>>> iprintf() isn't available. Is there a sensible place to add
information
>>> about which library functions are available for a particular
target?
>> I'd suggest adding a bool argument (HasIPrintf) to the
createSimplifyLibCallsPass function and the pass constructor.  Then clang (or
whatever is setting up the pass manager) can query the appropriate target info
to pass down this flag.
Sorry to jump into something midstream, but I just happened to read the 
above paragraph and thought I should comment.  Adding a boolean argument 
to a pass's constructor method is one of the things I have learned not 
to do.  If I ever give a "How Not to Write an LLVM Pass" talk, this
will
be one of the things in the list.

The problem with boolean default arguments in pass constructors is that 
they make a pass more difficult to use within the opt and bugpoint 
tools.  Inevitably you'll find some bug that only occurs when the flag 
is set to a non-default value, you'll fire up bugpoint, pass in the 
arguments printed by your tool's -debug-pass=Arguments option, and then 
wonder why the bug doesn't trigger.  Eventually you find out that it's 
due to the boolean flag you put into the constructor not being set the 
way you want.  You then have to manually change the constructor in the 
source code, recompile, run bugpoint, and then remember to change it 
back before committing your fix.

Perhaps you can escape this problem because you're working on a pass 
that is hard-coded into opt and bugpoint, but it'll only work if opt and 
bugpoint and whatever special tool that this pass is incorporated into 
all set that boolean in the same way under the same conditions.  If 
someone uses it in a tool that sets it using some non-standard criteria, 
then you run into this problem.

We went through this with Automatic Pool Allocation and SAFECode: it was 
one of several things that made bugpoint inconvenient or impossible to use.

I have not actually fixed this problem in Poolalloc/SAFECode yet, but 
I've got the following ideas to try:

1) Create one version of your pass that sets the boolean flag to true 
and another version of your pass that sets the boolean flag to false.  
You can probably use C++ templates or inheritance to do this.  Register 
each pass as a separate pass and have your tool pick which pass to run 
based upon whatever conditions you want.

For example, the Poolalloc pass has to use either EQTDDataStructures or 
EQBUDataStructures pass depending on whether it is used for SAFECode.  
Right now it picks one based on a boolean constructor.  Instead, I would 
have the Poolalloc pass be a template that takes the name of the pass to 
use as input and create two passes:Poolalloc<EQTDDataStructures> and 
Poolalloc<EQBUDataStructures>.  SAFECode would use 
Poolalloc<EQTDDataStructures>.

2) Create an analysis group that tells your pass what to do (i.e., your 
pass calls a method on the analysis group to get the boolean flag).  
Create two immutable passes belonging to the group and return different 
values for the boolean flag.

Either of these techniques will ensure that any PassManager based tool 
can print out a string with -debug-pass=Arguments that can be fed into 
opt or bugpoint to reproduce the same behavior.

My 4 cents.

-- John T.
>> Longer term, I'd like to introduce a simple interface (via
TargetRegistry) for exposing target libcall info.  This would be useful to
eliminate the HAVE_FLOORF etc #defines in simplifylibcalls, which break cross
compilation.
> Hi Richard,
>
> Just to close the loop on this, I went ahead and added a new
llvm/Target/TargetLibraryInfo.h and added it to simplifylibcalls. 
TargetLibraryInfo doesn't handle all the libcalls that SimplifyLibCalls
handles yet, but it should provide a starting point for iprintf: just add an
iprintf enum to TargetLibraryInfo and make your transformation predicated on
TLI->has(LibFunc::iprintf).
>
> It would be great to see this go into mainline,
>
> -Chris
>
>
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev

On Feb 24, 2011, at 12:41 AM, John Criswell wrote:
>>>> At the moment I've got this working locally by adding code
to the
>>>> simplify-libcalls pass. However this will break on targets
where
>>>> iprintf() isn't available. Is there a sensible place to add
information
>>>> about which library functions are available for a particular
target?
>>> I'd suggest adding a bool argument (HasIPrintf) to the
createSimplifyLibCallsPass function and the pass constructor.  Then clang (or
whatever is setting up the pass manager) can query the appropriate target info
to pass down this flag.
> 
> Sorry to jump into something midstream, but I just happened to read the 
> above paragraph and thought I should comment.  Adding a boolean argument 
> to a pass's constructor method is one of the things I have learned not 
> to do.  If I ever give a "How Not to Write an LLVM Pass" talk,
this will
> be one of the things in the list.
FWIW, the patch didn't do that.  SimplifyLibcalls depends on
TargetLibraryInfo, which is an analysis pass.

-Chris

On 24/02/11 07:21, Chris Lattner wrote:
> Hi Richard,
>
> Just to close the loop on this, I went ahead and added a new
llvm/Target/TargetLibraryInfo.h and added it to simplifylibcalls. 
TargetLibraryInfo doesn't handle all the libcalls that SimplifyLibCalls
handles yet, but it should provide a starting point for iprintf: just add an
iprintf enum to TargetLibraryInfo and make your transformation predicated on
TLI->has(LibFunc::iprintf).
>
> It would be great to see this go into mainline,
>
> -Chris
>    
Hi Chris,

Thanks for this - it looks to be exactly what I need. I'll should have 
some time next week to put together a patch with the iprintf optimizations.

-- 
Richard Osborne | XMOS
http://www.xmos.com