[llvm-commits] [PATCH] Move function prototype attribute inference to instcombine

Meador Inge meadori at codesourcery.com
Sun Dec 2 11:40:25 PST 2012


On Dec 2, 2012, at 1:30 PM, Benjamin Kramer wrote:

> 
> On 02.12.2012, at 20:06, Meador Inge <meadori at codesourcery.com> wrote:
> 
>> 
>> On Dec 2, 2012, at 12:25 PM, Benjamin Kramer wrote:
>> 
>>>> Ah, good point -- I didn't fully notice that pass.  I have moved the
>>>> attribute inference logic to the functionattrs pass.  The main difference
>>>> is that the library call attribute inference won't happen when '-fno-unit-at-a-time'
>>>> is thrown (I suppose that is OK).  Does the attached look OK?
>>> 
>>> I don't think it is safe to add attributes without asking TargetLibraryInfo if the function is available first. Can you add a  check?
>> 
>> Adding the check is easy enough, but the function set being checked is a superset
>> of what is available in TLI (e.g. strxfrm, stat, system, rmdir, rewind, bzero,
>> unlink, htonl, … to list a few).
>> 
>> Also, we are already currently doing this without checking TLI.  Although, that
>> is not an argument to continue doing it (if it is wrong, it is wrong), but I am
>> curious why we ever deemed it OK before.  Maybe it was an oversight?
>> 
>> In any case, if the right thing to do is add the TLI check and add the missing
>> functions to TLI, then that is fine, but I don't want to hold this patch up
>> on that work since all I am really doing here is moving code.  I will address
>> the TLI concerns in another patch.
> 
> The difference is that SimplifyLibcalls is disabled in freestanding environments, FunctionAttrs and InstCombine are not, so you're creating a regression here. As you know this was the sole reason for splitting out the SimplifyLibcalls pass in the first place, and I'm very happy that you're working hard on fixing this historical wart.
> 
> Adding the missing functions to TLI is the right way to proceed. It also lets you replace the string compares with a switch on the enum value from TLI, that's slightly nicer in my opinion.

I see.  I will get the TLI modifications in place first then.  Thanks for the review!

--
Meador Inge
CodeSourcery / Mentor Embedded




More information about the llvm-commits mailing list