<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;">Thanks Richard! Committed in r185049.<div><br><div><div>On Jun 26, 2013, at 5:10 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;">On Tue, Jun 25, 2013 at 9:16 PM, Michael Gottesman <<a href="mailto:mgottesman@apple.com">mgottesman@apple.com</a>> wrote:<br><blockquote type="cite"><br>On Jun 25, 2013, at 5:56 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> wrote:<br><br>Hi!<br><br>On Tue, Jun 25, 2013 at 5:34 PM, Michael Gottesman <<a href="mailto:mgottesman@apple.com">mgottesman@apple.com</a>><br>wrote:<br><br>Hey Richard!<br><br>Here is the promised patch. I took your original patch and made the<br>following changes:<br><br>Actual Changes:<br>1. In order to make hasFnAttr(Attribute::NoBuiltin) assert like Chris<br>requested, I had to extract out its implementation into a separate function<br>called hasFnAttrImpl. The reason for this is that in isNoBuiltin, I can not<br>use hasFnAttr(Attribute::NoBuiltin) to check for NoBuiltin since that would<br>cause the assertion = p. I need to access said functionality therein, so it<br>made sense to add a private helper method. Thus instead in isNoBuiltin I<br>just invoke the hasFnAttrImpl function and in hasFnAttr I perform the assert<br>and then call hasFnAttrImpl yielding the proper behavior.<br><br><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><br><br>Ok. I just left mine in since it was already done.<br><br><br>2. Added code to lib/IR/Verifier.cpp to ensure that we do not accept the<br>builtin attribute on function declarations/definitions.<br>Tests:<br>1. I added a test that tests that we accept builtin attributes on function<br>call sites where the function has a nobuiltin attribute. It also tests that<br>we can assembly/disassemble said builtin.<br>2. I added a test that tests that llvm-as fails if we have a builtin<br>attribute on a function declaration.<br>3. I added a test that tests that llvm-as fails if we have a builtin<br>attribute on a function definition.<br>3. I added a test that tests that llvm-as fails if we have a builtin<br>attribute on a call site for a function that does not have the nobuiltin<br>attribute.<br><br><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><br><br>Done.<br><br><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><br><br>Done.<br><br>Hows this look?<br></blockquote><br>Looks good to me, thanks!</div></blockquote></div><br></div></body></html>