On Fri, May 15, 2015 at 9:53 AM, Teresa Johnson <tejohnson at google.com> wrote:> On Fri, May 15, 2015 at 9:20 AM, David Blaikie <dblaikie at gmail.com> wrote: > > > > > > On Fri, May 15, 2015 at 9:18 AM, David Blaikie <dblaikie at gmail.com> > wrote: > >>> > >>> > >>> >> - Marking of imported functions (for use in ThinLTO-specific symbol > >>> >> linking and global DCE, for example). > >>> > > >>> > Marking how? Do you mean giving them internal linkage, or something > >>> > else? > >>> > >>> Mentioned just after this: either an in-memory flag on the Function > >>> class, or potentially in the IR. For the prototype I just had a flag > >>> on the Function class. > >> > >> > >> Would this be anything other than "available externally" linkage? > (either > >> this module is responsible for emitting the definition of this function > for > >> other modules to call, or it's not - if it's not, either this module > depends > >> on another module to provide a definition (this would be "available > >> externally") or it doesn't (this would be "internal" linkage")) - I > think... > >> though I'm sort of jumping into the middle of some of this & might've > missed > >> some context, sorry. > > > > > > & I see your follow up further down in that thread: > > > > "There were only a couple minor tweaks required here (under the flag I > > added to the Function indicating that this was imported). Only > > promoted statics are remarked available_externally. For a > > non-discardable symbol that was imported, we can discard here since we > > are done with inlining (it is non-discardable in its home module)." > > > > &, like Duncan, I'll wait for more details on that front. (may or may > not be > > useful to split some of these subthreads into separate email threads to > keep > > discussion clear - but I'm not sure) > > I just went back and looked at my prototype and I had remembered this > wrong. An imported function is always marked > AvailableExternallyLinkage, unless it has link once linkage. > > As far as using that to indicate that it is an aux function, I was > concerned about overloading the meaning of that linkage type. See the > next para for an example where I am unsure about doing this... > > Looking back through my GlobalDCE changes, it looks like one of the > places I had changed (where we mark defined globals in runOnModule) > already has a guard for !hasAvailableExternallyLinkage and > !isDiscardableIfUnused, so my additional guard against marking > imported functions is unnecessary. But the other place I had to change > was in GlobalIsNeeded where it walks through the function and > recursively marks any referenced global as needed. Here there was no > guard against marking a global that is available externally as needed > if it is referenced. I had added a check here to not mark imported > functions as needed on reference unless they were discardable (i.e. > link once). Is this a bug - should this have a guard against marking > available externally function refs as needed? >Duncan's probably got a better idea of what the right answer is here. I suspect "yes". The trick with available_externally is to ensure we keep these around for long enough that their definitions are useful (for inlining, constant prop, all that good stuff) but remove them before we actually do too much work on them, or at least before we emit them into the final object file. I imagine if GlobalDCE isn't removing available_externally functions it's because they're still useful in the optimization pipeline and something further down the pipe removes them (because they do, ultimately, get removed). Any idea what problematic behavior you were seeing that was addressed by changing DCE to remove these functions was? I imagine whatever the bad behavior was, it could be demonstrated without ThinLTO and with a normal module with an available_externally function & would be worth discussing what the right behavior is in that context, regardless of ThinLTO.> > There was one other change to GlobalDCE that I had to make, where we > erase the list of DeadFunctions from the module. In my case we may > have a function body that was eliminated, but still have references to > it (i.e. in the case I am talking about above). In that case it is now > just a declaration and we don't erase it from the function list on the > module. If the GlobalsIsNeeded code is changed to avoid marking > available externally function refs as needed then this change would be > needed for that case as well. > > Thanks, > Teresa > > > > >> > >> > >> > >> - David > > > > > > > > -- > Teresa Johnson | Software Engineer | tejohnson at google.com | 408-460-2413 >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20150515/d67c472e/attachment.html>
On Fri, May 15, 2015 at 10:04 AM, David Blaikie <dblaikie at gmail.com> wrote:> > > On Fri, May 15, 2015 at 9:53 AM, Teresa Johnson <tejohnson at google.com> > wrote: >> >> On Fri, May 15, 2015 at 9:20 AM, David Blaikie <dblaikie at gmail.com> wrote: >> > >> > >> > On Fri, May 15, 2015 at 9:18 AM, David Blaikie <dblaikie at gmail.com> >> > wrote: >> >>> >> >>> >> >>> >> - Marking of imported functions (for use in ThinLTO-specific symbol >> >>> >> linking and global DCE, for example). >> >>> > >> >>> > Marking how? Do you mean giving them internal linkage, or something >> >>> > else? >> >>> >> >>> Mentioned just after this: either an in-memory flag on the Function >> >>> class, or potentially in the IR. For the prototype I just had a flag >> >>> on the Function class. >> >> >> >> >> >> Would this be anything other than "available externally" linkage? >> >> (either >> >> this module is responsible for emitting the definition of this function >> >> for >> >> other modules to call, or it's not - if it's not, either this module >> >> depends >> >> on another module to provide a definition (this would be "available >> >> externally") or it doesn't (this would be "internal" linkage")) - I >> >> think... >> >> though I'm sort of jumping into the middle of some of this & might've >> >> missed >> >> some context, sorry. >> > >> > >> > & I see your follow up further down in that thread: >> > >> > "There were only a couple minor tweaks required here (under the flag I >> > added to the Function indicating that this was imported). Only >> > promoted statics are remarked available_externally. For a >> > non-discardable symbol that was imported, we can discard here since we >> > are done with inlining (it is non-discardable in its home module)." >> > >> > &, like Duncan, I'll wait for more details on that front. (may or may >> > not be >> > useful to split some of these subthreads into separate email threads to >> > keep >> > discussion clear - but I'm not sure) >> >> I just went back and looked at my prototype and I had remembered this >> wrong. An imported function is always marked >> AvailableExternallyLinkage, unless it has link once linkage. >> >> As far as using that to indicate that it is an aux function, I was >> concerned about overloading the meaning of that linkage type. See the >> next para for an example where I am unsure about doing this... >> >> Looking back through my GlobalDCE changes, it looks like one of the >> places I had changed (where we mark defined globals in runOnModule) >> already has a guard for !hasAvailableExternallyLinkage and >> !isDiscardableIfUnused, so my additional guard against marking >> imported functions is unnecessary. But the other place I had to change >> was in GlobalIsNeeded where it walks through the function and >> recursively marks any referenced global as needed. Here there was no >> guard against marking a global that is available externally as needed >> if it is referenced. I had added a check here to not mark imported >> functions as needed on reference unless they were discardable (i.e. >> link once). Is this a bug - should this have a guard against marking >> available externally function refs as needed? > > > Duncan's probably got a better idea of what the right answer is here. I > suspect "yes". > > The trick with available_externally is to ensure we keep these around for > long enough that their definitions are useful (for inlining, constant prop, > all that good stuff) but remove them before we actually do too much work on > them, or at least before we emit them into the final object file.Yep, and that is exactly how long I want my imported functions to stick around. Currently I have the ThinLTO import pass added at the start of addLTOOptimizationPasses, just after the AA passes. So the imported functions stick around through global opt, constant merge, combining, and inlining. The call to GlobalDCE is just after inlining and a second globalopt.> > I imagine if GlobalDCE isn't removing available_externally functions it's > because they're still useful in the optimization pipeline and something > further down the pipe removes them (because they do, ultimately, get > removed).That would be good to know, since if they are useful later on in the pipe then presumably my imported functions could be as well. But I wonder who is removing them since GlobalDCE isn't (if there are refs)?> Any idea what problematic behavior you were seeing that was > addressed by changing DCE to remove these functions was? I imagine whatever > the bad behavior was, it could be demonstrated without ThinLTO and with a > normal module with an available_externally function & would be worth > discussing what the right behavior is in that context, regardless of > ThinLTO.So I never changed GlobalDCE to eliminate available externally functions in general, because I came at this with the view that I shouldn't rely on just the linkage info. But I can certainly try that experiment. What isn't clear to me is what all uses the available externally linkage type currently - do you happen to know? Thanks, Teresa> >> >> >> There was one other change to GlobalDCE that I had to make, where we >> erase the list of DeadFunctions from the module. In my case we may >> have a function body that was eliminated, but still have references to >> it (i.e. in the case I am talking about above). In that case it is now >> just a declaration and we don't erase it from the function list on the >> module. If the GlobalsIsNeeded code is changed to avoid marking >> available externally function refs as needed then this change would be >> needed for that case as well. >> >> Thanks, >> Teresa >> >> > >> >> >> >> >> >> >> >> - David >> > >> > >> >> >> >> -- >> Teresa Johnson | Software Engineer | tejohnson at google.com | 408-460-2413 > >-- Teresa Johnson | Software Engineer | tejohnson at google.com | 408-460-2413
> On 2015-May-15, at 13:15, Teresa Johnson <tejohnson at google.com> wrote: > > What isn't clear to me is what all uses the available > externally linkage type currently - do you happen to know?It's used for the `inline` keyword in the C language. If you do a `git grep available_externally -- test/` inside a clang checkout you might find some other uses. $ cat available-externally.c inline int foo(int i) { return i; } int bar(int i) { return foo(i); } $ clang -S -emit-llvm -O1 available-externally.c -o - ; ModuleID = 'available-externally.c' target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128" target triple = "x86_64-apple-macosx10.10.0" ; Function Attrs: nounwind readnone ssp uwtable define i32 @bar(i32 %i) #0 { %1 = tail call i32 @foo(i32 %i) ret i32 %1 } ; Function Attrs: inlinehint nounwind readnone ssp uwtable define available_externally i32 @foo(i32 %i) #1 { ret i32 %i } attributes #0 = { nounwind readnone ssp uwtable "less-precise-fpmad"="false" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="core2" "unsafe-fp-math"="false" "use-soft-float"="false" } attributes #1 = { inlinehint nounwind readnone ssp uwtable "less-precise-fpmad"="false" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="core2" "unsafe-fp-math"="false" "use-soft-float"="false" } !llvm.module.flags = !{!0} !llvm.ident = !{!1} !0 = !{i32 1, !"PIC Level", i32 2} !1 = !{!"Apple LLVM version 7.0.0 (clang-700.9.38.2)"}
On Fri, May 15, 2015 at 1:15 PM, Teresa Johnson <tejohnson at google.com> wrote:> On Fri, May 15, 2015 at 10:04 AM, David Blaikie <dblaikie at gmail.com> > wrote: > > > > > > On Fri, May 15, 2015 at 9:53 AM, Teresa Johnson <tejohnson at google.com> > > wrote: > >> > >> On Fri, May 15, 2015 at 9:20 AM, David Blaikie <dblaikie at gmail.com> > wrote: > >> > > >> > > >> > On Fri, May 15, 2015 at 9:18 AM, David Blaikie <dblaikie at gmail.com> > >> > wrote: > >> >>> > >> >>> > >> >>> >> - Marking of imported functions (for use in ThinLTO-specific > symbol > >> >>> >> linking and global DCE, for example). > >> >>> > > >> >>> > Marking how? Do you mean giving them internal linkage, or > something > >> >>> > else? > >> >>> > >> >>> Mentioned just after this: either an in-memory flag on the Function > >> >>> class, or potentially in the IR. For the prototype I just had a flag > >> >>> on the Function class. > >> >> > >> >> > >> >> Would this be anything other than "available externally" linkage? > >> >> (either > >> >> this module is responsible for emitting the definition of this > function > >> >> for > >> >> other modules to call, or it's not - if it's not, either this module > >> >> depends > >> >> on another module to provide a definition (this would be "available > >> >> externally") or it doesn't (this would be "internal" linkage")) - I > >> >> think... > >> >> though I'm sort of jumping into the middle of some of this & might've > >> >> missed > >> >> some context, sorry. > >> > > >> > > >> > & I see your follow up further down in that thread: > >> > > >> > "There were only a couple minor tweaks required here (under the flag I > >> > added to the Function indicating that this was imported). Only > >> > promoted statics are remarked available_externally. For a > >> > non-discardable symbol that was imported, we can discard here since we > >> > are done with inlining (it is non-discardable in its home module)." > >> > > >> > &, like Duncan, I'll wait for more details on that front. (may or may > >> > not be > >> > useful to split some of these subthreads into separate email threads > to > >> > keep > >> > discussion clear - but I'm not sure) > >> > >> I just went back and looked at my prototype and I had remembered this > >> wrong. An imported function is always marked > >> AvailableExternallyLinkage, unless it has link once linkage. > >> > >> As far as using that to indicate that it is an aux function, I was > >> concerned about overloading the meaning of that linkage type. See the > >> next para for an example where I am unsure about doing this... > >> > >> Looking back through my GlobalDCE changes, it looks like one of the > >> places I had changed (where we mark defined globals in runOnModule) > >> already has a guard for !hasAvailableExternallyLinkage and > >> !isDiscardableIfUnused, so my additional guard against marking > >> imported functions is unnecessary. But the other place I had to change > >> was in GlobalIsNeeded where it walks through the function and > >> recursively marks any referenced global as needed. Here there was no > >> guard against marking a global that is available externally as needed > >> if it is referenced. I had added a check here to not mark imported > >> functions as needed on reference unless they were discardable (i.e. > >> link once). Is this a bug - should this have a guard against marking > >> available externally function refs as needed? > > > > > > Duncan's probably got a better idea of what the right answer is here. I > > suspect "yes". > > > > The trick with available_externally is to ensure we keep these around for > > long enough that their definitions are useful (for inlining, constant > prop, > > all that good stuff) but remove them before we actually do too much work > on > > them, or at least before we emit them into the final object file. > > Yep, and that is exactly how long I want my imported functions to > stick around. Currently I have the ThinLTO import pass added at the > start of addLTOOptimizationPasses, just after the AA passes. So the > imported functions stick around through global opt, constant merge, > combining, and inlining. The call to GlobalDCE is just after inlining > and a second globalopt. > > > > > I imagine if GlobalDCE isn't removing available_externally functions it's > > because they're still useful in the optimization pipeline and something > > further down the pipe removes them (because they do, ultimately, get > > removed). > > That would be good to know, since if they are useful later on in the > pipe then presumably my imported functions could be as well. But I > wonder who is removing them since GlobalDCE isn't (if there are refs)? >Yep - seems no one removes them (well, probably if there are no calls to the function at all they get removed as usual (eg: if all the calls are inlined we probably drop the function as we would for a linkonce_odr function)). If they're still around come codegen time, we just skip them: lib/CodeGen/MachineFunctionPass.cpp, MachineFunctionPass::runOnFunction: 33| bool MachineFunctionPass::runOnFunction(Function &F) { 34| // Do not codegen any 'available_externally' functions at all, they have 35| // definitions outside the translation unit. 36| if (F.hasAvailableExternallyLinkage()) 37| return false; 38| 39| MachineFunction &MF = getAnalysis<MachineFunctionAnalysis>().getMF(); 40| return runOnMachineFunction(MF); 41| }> > > Any idea what problematic behavior you were seeing that was > > addressed by changing DCE to remove these functions was? I imagine > whatever > > the bad behavior was, it could be demonstrated without ThinLTO and with a > > normal module with an available_externally function & would be worth > > discussing what the right behavior is in that context, regardless of > > ThinLTO. > > So I never changed GlobalDCE to eliminate available externally > functions in general, because I came at this with the view that I > shouldn't rely on just the linkage info. But I can certainly try that > experiment. What isn't clear to me is what all uses the available > externally linkage type currently - do you happen to know? > > Thanks, > Teresa > > > > >> > >> > >> There was one other change to GlobalDCE that I had to make, where we > >> erase the list of DeadFunctions from the module. In my case we may > >> have a function body that was eliminated, but still have references to > >> it (i.e. in the case I am talking about above). In that case it is now > >> just a declaration and we don't erase it from the function list on the > >> module. If the GlobalsIsNeeded code is changed to avoid marking > >> available externally function refs as needed then this change would be > >> needed for that case as well. > >> > >> Thanks, > >> Teresa > >> > >> > > >> >> > >> >> > >> >> > >> >> - David > >> > > >> > > >> > >> > >> > >> -- > >> Teresa Johnson | Software Engineer | tejohnson at google.com | > 408-460-2413 > > > > > > > > -- > Teresa Johnson | Software Engineer | tejohnson at google.com | 408-460-2413 >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20150515/a6b2dcf9/attachment.html>