On Feb 19, 2013, at 10:18 AM, Chris Lattner <clattner at apple.com> wrote:> On Feb 18, 2013, at 10:31 PM, Bill Wendling <wendling at apple.com> wrote: > >>>>> >>>>> In the context of LTO, it makes sense for the attribute to be on function bodies, not on prototypes. >>>>> >>>> Yeah, I noticed that after sending this patch. I modified it to check the function CI is in for that attribute. >>> >>> Was that in the patch you committed? What specific patch are you looking for? >>> >> Yeah, that was in the one that I committed. I basically want something like this: >> >> define void @foo() "no-builtin" { >> call void @printf() >> } >> >> And then the `call' to printf would check for the "no-builtin" attribute on `@foo' to determine if it can convert it to `puts'. Having the attribute on the declaration of `printf' doesn't help us here. And I'm not sure if having it on the call would be beneficial either. > > Ok, this makes a lot of sense to me. > >>>> Once we have support for the `-fno-builtin-FUNCTION' flag, I expect the attribute to look something like this: >>>> >>>> "no-builtin-functions" = "puts,foo,bar" >>> >>> I guess this could work, this means that simplifylibcalls (and others) would check this instead of TargetLibraryInfo? >>> >> Yes. > > Sounds good, much of TLI can just disappear when this happens, woot. >I'm hoping the whole of TLI can disappear with the new attributes rewrite. :)>>> Out of curiosity, how important is -fno-builtin-foo? Isn't it enough to just have -fno-builtin-foo disable *all* builtin optimizations for the unit of code being compiled? This is overly conservative - is that actually a problem for something? >>> >> Being overly conservative will of course keep the correct semantics for the program. I suppose that there could be some people who have their own special version of a builtin that they want to use, but they don't want all builtin optimizations to be disabled. > > I guess I'm just asking "how much do we care" about that use case? Is it worth uglifying IR dumps? >There are two (old) bugs related to this: a PR and a radar. They both seem to be of low priority, though. One thing that might help in this discussion is to note that attributes are *not* part of the function type (signature, whatever). So we cannot have a function declaration with 'nobuiltin' and another declaration without it. Another thing to note is that during LTO I plan to restrict inlining of functions to those whose attributes match. This is an overly conservative approach, but one which will minimize generating code the user didn't want/expect. This restriction is meant to be relaxed over time on a case-by-case basis. In general though, I expect most TUs for a project will be compiled with the same command line flags, producing the same attributes for most functions. So if `@foo' is marked `nobuiltin' and `@bar' isn't marked with `nobuiltin', then `@foo' will not be inlined into `@bar' and vice versa. Here's what I propose then: Note: I'm intentionally leaving out the `-fno-builtin-FUNCTION' flag, because clang doesn't yet support it, and that can be easily added later --- probably in the form I mentioned above. I add the `NoBuiltin' attribute to the Attribute::AttrKind enum list and a `nobuiltin' keyword to the LLVM assembly language (documentation, etc.). This can be applied to function definitions, but not to function declarations or to call/invoke instructions, because that kind of granularity doesn't make a lot of sense to me. I want the attribute to be in the 'enum' list because string attributes are meant more for target-dependent attributes rather than IR-level attributes. (The distinction isn't set in stone, but I feel that this doesn't need to be a string.) The LibSimplify code will check the function containing the call/invoke for the `NoBuiltin' attribute to see if it is allowed to simplify the builtin. What do you think? -bw
On Feb 19, 2013, at 1:17 PM, Bill Wendling <wendling at apple.com> wrote:>>>>> Once we have support for the `-fno-builtin-FUNCTION' flag, I expect the attribute to look something like this: >>>>> >>>>> "no-builtin-functions" = "puts,foo,bar" >>>> >>>> I guess this could work, this means that simplifylibcalls (and others) would check this instead of TargetLibraryInfo? >>>> >>> Yes. >> >> Sounds good, much of TLI can just disappear when this happens, woot. >> > I'm hoping the whole of TLI can disappear with the new attributes rewrite. :)That would be nice, but I don't think that will happen 100%. We still need to know whether "memset pattern" exists, for example. I'm happy with it getting dramatically downsized :)>>>> Out of curiosity, how important is -fno-builtin-foo? Isn't it enough to just have -fno-builtin-foo disable *all* builtin optimizations for the unit of code being compiled? This is overly conservative - is that actually a problem for something? >>>> >>> Being overly conservative will of course keep the correct semantics for the program. I suppose that there could be some people who have their own special version of a builtin that they want to use, but they don't want all builtin optimizations to be disabled. >> >> I guess I'm just asking "how much do we care" about that use case? Is it worth uglifying IR dumps? >> > There are two (old) bugs related to this: a PR and a radar. They both seem to be of low priority, though. > > One thing that might help in this discussion is to note that attributes are *not* part of the function type (signature, whatever). So we cannot have a function declaration with 'nobuiltin' and another declaration without it.You mean "So we *can* have a function declaration"...> Another thing to note is that during LTO I plan to restrict inlining of functions to those whose attributes match. This is an overly conservative approach, but one which will minimize generating code the user didn't want/expect.This is exactly why these should be put on calls in the compiled scope, not on the function bodies being compiled, which I'm pretty sure your patch doesn't do.> Note: I'm intentionally leaving out the `-fno-builtin-FUNCTION' flag, because clang doesn't yet support it, and that can be easily added later --- probably in the form I mentioned above.Right, sounds like a great second step if anyone ever cares enough to implement it.> I add the `NoBuiltin' attribute to the Attribute::AttrKind enum list and a `nobuiltin' keyword to the LLVM assembly language (documentation, etc.).Sounds good.> This can be applied to function definitions, but not to function declarations or to call/invoke instructions, because that kind of granularity doesn't make a lot of sense to me.I think we should do it, otherwise inlining is unnecessarily harmed. Given that attributes are displayed in .ll files with the # syntax, there should be no bloat/unreadability impact.> The LibSimplify code will check the function containing the call/invoke for the `NoBuiltin' attribute to see if it is allowed to simplify the builtin. > What do you think?Sounds ok, but I think the attribute should be on compiled call sites instead of compiled function bodies. If we're going to do this, might as well do it right. Thanks Bill, -Chris
On Feb 19, 2013, at 9:15 PM, Chris Lattner <clattner at apple.com> wrote:>> There are two (old) bugs related to this: a PR and a radar. They both seem to be of low priority, though. >> >> One thing that might help in this discussion is to note that attributes are *not* part of the function type (signature, whatever). So we cannot have a function declaration with 'nobuiltin' and another declaration without it. > > You mean "So we *can* have a function declaration"... >I'm confused. That's not something that works today. This doesn't assemble: declare i32 @x() nounwind declare i32 @x() noinline define i32 @foo(i32 %a, i32 %b, i32 %c) nounwind { %t0 = tail call i32 @x() nounwind %t1 = sdiv i32 %a, %b ret i32 %t1 } Do you want this to change?>> Another thing to note is that during LTO I plan to restrict inlining of functions to those whose attributes match. This is an overly conservative approach, but one which will minimize generating code the user didn't want/expect. > > This is exactly why these should be put on calls in the compiled scope, not on the function bodies being compiled, which I'm pretty sure your patch doesn't do. >Okay. I was confused by the previous discussion then.>> This can be applied to function definitions, but not to function declarations or to call/invoke instructions, because that kind of granularity doesn't make a lot of sense to me. > > I think we should do it, otherwise inlining is unnecessarily harmed. Given that attributes are displayed in .ll files with the # syntax, there should be no bloat/unreadability impact. > >> The LibSimplify code will check the function containing the call/invoke for the `NoBuiltin' attribute to see if it is allowed to simplify the builtin. >> What do you think? > > Sounds ok, but I think the attribute should be on compiled call sites instead of compiled function bodies. If we're going to do this, might as well do it right. >Alright, then the attribute will be applied to the call/invoke instructions and not the function definitions and declarations. -bw