NoBuiltin Attribute Patch

[Michael Gottesman]

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?

Michael


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130625/87f0f835/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Added-support-for-the-Builtin-attribute.patch
Type: application/octet-stream
Size: 20466 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130625/87f0f835/attachment.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130625/87f0f835/attachment-0001.html>