On Feb 18, 2013, at 6:10 PM, Chris Lattner <clattner at apple.com> wrote:> On Feb 18, 2013, at 4:49 PM, Bill Wendling <wendling at apple.com> wrote: >>> Hi Bill, >>> >>> I think the concept of this patch makes sense, but the implementation does not. >>> >>> I have: >>> >>> void foo() { >>> printf("hello\n"); >>> } >>> >>> and I build with -fno-builtin-puts. If I understand correctly, *foo* will be marked with "nobuiltin", but this code in simplifylibcalls looks at the printf: >>> >>> Value *LibCallSimplifier::optimizeCall(CallInst *CI) { >>> + Function *F = CI->getCalledFunction(); >>> + if (F->hasFnAttribute(Attribute::NoBuiltin)) return 0; >>> return Impl->optimizeCall(CI); >>> } >>> >>> 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.>> 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.> 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. -bw
On 2/19/2013 12:31 AM, Bill Wendling wrote:> > 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.Having the attribute on the prototype of printf would tell us that calls to printf cannot be converted to calls to anything else. Isn't that what you want? If you want to prevent the optimizations on foo that take advantage of the knowledge about certain known functions, then we should use a different attribute for that, or else things can get really confusing. -Krzysztof -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
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.>> 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? -Chris
On Feb 19, 2013, at 7:46 AM, Krzysztof Parzyszek <kparzysz at codeaurora.org> wrote:> On 2/19/2013 12:31 AM, Bill Wendling wrote: >> >> 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. > > Having the attribute on the prototype of printf would tell us that calls to printf cannot be converted to calls to anything else. Isn't that what you want?No, it isn't because that isn't sufficient. Besides the example I gave in the other email to this thread, consider things like: void foo() { auto fp = printf; fp("xyz\n"); } With -fno-builtin-printf, we can't optimize the call to printf, even though it only becomes apparent after (trivial) devirtualization. -Chris> > If you want to prevent the optimizations on foo that take advantage of the knowledge about certain known functions, then we should use a different attribute for that, or else things can get really confusing. > > -Krzysztof > > -- > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
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