NoBuiltin Attribute Patch

[Michael Gottesman]

Thanks Richard! Committed in r185049.

On Jun 26, 2013, at 5:10 PM, Richard Smith <richard at metafoo.co.uk> wrote:

> On Tue, Jun 25, 2013 at 9:16 PM, Michael Gottesman <mgottesman at apple.com> wrote:
>> 
>> On Jun 25, 2013, at 5:56 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>> 
>> Hi!
>> 
>> On Tue, Jun 25, 2013 at 5:34 PM, Michael Gottesman <mgottesman at apple.com>
>> wrote:
>> 
>> Hey Richard!
>> 
>> Here is the promised patch. I took your original patch and made the
>> following changes:
>> 
>> Actual Changes:
>> 1. In order to make hasFnAttr(Attribute::NoBuiltin) assert like Chris
>> requested, I had to extract out its implementation into a separate function
>> called hasFnAttrImpl. The reason for this is that in isNoBuiltin, I can not
>> use hasFnAttr(Attribute::NoBuiltin) to check for NoBuiltin since that would
>> cause the assertion = p. I need to access said functionality therein, so it
>> made sense to add a private helper method. Thus instead in isNoBuiltin I
>> just invoke the hasFnAttrImpl function and in hasFnAttr I perform the assert
>> and then call hasFnAttrImpl yielding the proper behavior.
>> 
>> 
>> Sorry, it looks like I must not have sent out the most recent version
>> of the patch. That's attached; I took a slightly different approach
>> for this, and did some refactoring of lib/IR/Instructions.cpp to clean
>> it up. I think it turned out pretty nicely, and removes some
>> pre-existing code duplication, but either approach seems OK to me.
>> 
>> 
>> Ok. I just left mine in since it was already done.
>> 
>> 
>> 2. Added code to lib/IR/Verifier.cpp to ensure that we do not accept the
>> builtin attribute on function declarations/definitions.
>> Tests:
>> 1. I added a test that tests that we accept builtin attributes on function
>> call sites where the function has a nobuiltin attribute. It also tests that
>> we can assembly/disassemble said builtin.
>> 2. I added a test that tests that llvm-as fails if we have a builtin
>> attribute on a function declaration.
>> 3. I added a test that tests that llvm-as fails if we have a builtin
>> attribute on a function definition.
>> 3. I added a test that tests that llvm-as fails if we have a builtin
>> attribute on a call site for a function that does not have  the nobuiltin
>> attribute.
>> 
>> 
>> I'd prefer the -fail- tests to use FileCheck to check for a specific
>> flavor of failure. I'm worried that if we change the IR language so
>> that they no longer parse for some other reason, we'll lose test
>> coverage. (With that done, can you combine them into a single file?)
>> 
>> 
>> Done.
>> 
>> 
>> Also, we should have some tests that check that the 'builtin'
>> attribute works as an override to 'nobuiltin'. Maybe add a
>> simplify-lib-calls test that uses it?
>> 
>> 
>> Done.
>> 
>> Hows this look?
> 
> Looks good to me, thanks!

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130626/05afeef7/attachment.html>