<html><head><meta http-equiv="Content-Type" content="text/html charset=iso-8859-1"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><br><div><div>On Jun 25, 2013, at 5:56 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">Hi!<br><br>On Tue, Jun 25, 2013 at 5:34 PM, Michael Gottesman <<a href="mailto:mgottesman@apple.com">mgottesman@apple.com</a>> wrote:<br><blockquote type="cite">Hey Richard!<br><br>Here is the promised patch. I took your original patch and made the following changes:<br><br>Actual Changes:<br>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.<br></blockquote><br>Sorry, it looks like I must not have sent out the most recent version<br>of the patch. That's attached; I took a slightly different approach<br>for this, and did some refactoring of lib/IR/Instructions.cpp to clean<br>it up. I think it turned out pretty nicely, and removes some<br>pre-existing code duplication, but either approach seems OK to me.<br></div></blockquote><div dir="auto"><br></div><div dir="auto">Ok. I just left mine in since it was already done.</div><br><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><br><blockquote type="cite">2. Added code to lib/IR/Verifier.cpp to ensure that we do not accept the builtin attribute on function declarations/definitions.<br>Tests:<br>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.<br>2. I added a test that tests that llvm-as fails if we have a builtin attribute on a function declaration.<br>3. I added a test that tests that llvm-as fails if we have a builtin attribute on a function definition.<br>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.<br></blockquote><br>I'd prefer the -fail- tests to use FileCheck to check for a specific<br>flavor of failure. I'm worried that if we change the IR language so<br>that they no longer parse for some other reason, we'll lose test<br>coverage. (With that done, can you combine them into a single file?)<br></div></blockquote><div dir="auto"><br></div><div dir="auto">Done.</div><br><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><br>Also, we should have some tests that check that the 'builtin'<br>attribute works as an override to 'nobuiltin'. Maybe add a<br>simplify-lib-calls test that uses it?<br></div></blockquote><div dir="auto"><br></div><div dir="auto">Done.</div><div dir="auto"><br></div><div dir="auto">Hows this look?</div><div dir="auto"><br></div><div dir="auto">Michael</div><div dir="auto"><br></div><div dir="auto"></div></div></body></html>