NoBuiltin Attribute Patch

[Richard Smith]

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.

> 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?)

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?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: builtin-attr.diff
Type: application/octet-stream
Size: 16230 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130625/fd3c242e/attachment.obj>