NoBuiltin Attribute Patch

[Richard Smith]

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!