Issue 78780043: go.tools/go/ssa: add a flag for selecting gccgo-style s... (Closed)
Descriptiongo.tools/go/ssa: add a flag for selecting bare init functions
Bare init functions omit calls to dependent init functions and the
use of an init guard. They are useful in cases where the client uses
a different calling convention for init functions, or cases where
it is easier for a client to analyze bare init functions.
Patch Set 1 #Patch Set 2 : diff -r 95dec6840e7e https://code.google.com/p/go.tools #Patch Set 3 : diff -r 95dec6840e7e https://code.google.com/p/go.tools #Patch Set 4 : diff -r b6a3b105fbb0 https://code.google.com/p/go.tools #
Total comments: 4
Patch Set 5 : diff -r 7e8840964994 https://code.google.com/p/go.tools #
MessagesTotal messages: 19
What problem does this solve? There's no portable ABI for Go object code files.
Sign in to reply to this message.
On 2014/03/21 14:40:22, adonovan wrote: > What problem does this solve? There's no portable ABI for Go object code files. This change allows compiler clients to target the gccgo ABI (which is basically the platform C ABI). Now that I've thought about it some more I wonder if it would be better to create the synthesized init function this way by default. The client can always synthesize the guard check and the calls to the dependent import functions by itself if it needs to.
Sign in to reply to this message.
On 21 March 2014 13:36, <pcc@google.com> wrote: > On 2014/03/21 14:40:22, adonovan wrote: > >> What problem does this solve? There's no portable ABI for Go object >> > code files. > > This change allows compiler clients to target the gccgo ABI (which is > basically the platform C ABI). > But why does this matter? Are you expecting to link object files compiled by gccgo and object files compiled by your go/ssa-base compiler (llgo?) into the same image? I wouldn't expect that to work without imposing a number of additional requirements on the design. If all the objects files are produced by the same compiler (e.g. llgo) then it doesn't matter which scheme it uses, so long as it uses some scheme consistently.
Sign in to reply to this message.
On Fri, Mar 21, 2014 at 12:31 PM, Alan Donovan <adonovan@google.com> wrote: > On 21 March 2014 13:36, <pcc@google.com> wrote: > >> On 2014/03/21 14:40:22, adonovan wrote: >> >>> What problem does this solve? There's no portable ABI for Go object >>> >> code files. >> >> This change allows compiler clients to target the gccgo ABI (which is >> basically the platform C ABI). >> > > But why does this matter? Are you expecting to link object files compiled > by gccgo and object files compiled by your go/ssa-base compiler (llgo?) > into the same image? > Yes. > I wouldn't expect that to work without imposing a number of additional > requirements on the design. > Can you give an example of such a requirement? > If all the objects files are produced by the same compiler (e.g. llgo) > then it doesn't matter which scheme it uses, so long as it uses some scheme > consistently. > Sure. Later, we might want to introduce a new ABI for llgo that (among other things) uses go/ssa's existing style of init functions (I think this will be required in order to be able to dynamically load or JIT packages into a running process). But I also think it's possible to build something that is compatible with gccgo's libgo. Peter
Sign in to reply to this message.
On 21 March 2014 16:38, Peter Collingbourne <pcc@google.com> wrote: > > I wouldn't expect that to work without imposing a number of additional >> requirements on the design. >> > > Can you give an example of such a requirement? > Now that I think about it, it only requires that your code generator exactly match the gccgo ABI, but I suppose that's within your control. I don't think you need to make a change to go/ssa to implement this, though. All you need is a one-line check in your code generator that says: if the current function is a package initializer, and the current instruction is a call of another package initializer, treat it as a no-op.
Sign in to reply to this message.
On Fri, Mar 21, 2014 at 2:46 PM, Alan Donovan <adonovan@google.com> wrote: > On 21 March 2014 16:38, Peter Collingbourne <pcc@google.com> wrote: >> >> I wouldn't expect that to work without imposing a number of additional >>> requirements on the design. >>> >> >> Can you give an example of such a requirement? >> > > Now that I think about it, it only requires that your code generator > exactly match the gccgo ABI, but I suppose that's within your control. > > I don't think you need to make a change to go/ssa to implement this, > though. All you need is a one-line check in your code generator that says: > if the current function is a package initializer, and the current > instruction is a call of another package initializer, treat it as a no-op. > That would work for llgo, but what about the guard check? Sure, it's redundant for the gccgo ABI, but what if a client wants to replace it with some arbitrary other thing? For example, in a dynamic loading/JIT scenario, a client may want to use atomics to make the check thread-safe. Which is why I think the synthesized init function should only provide the init semantics for the current package and let the client decide how it should interact with the rest of the system. Peter
Sign in to reply to this message.
On 21 March 2014 18:06, Peter Collingbourne <pcc@google.com> wrote: > That would work for llgo, but what about the guard check? Sure, it's > redundant for the gccgo ABI, but what if a client wants to replace it with > some arbitrary other thing? For example, in a dynamic loading/JIT scenario, > a client may want to use atomics to make the check thread-safe. Which is > why I think the synthesized init function should only provide the init > semantics for the current package and let the client decide how it should > interact with the rest of the system. > Let's call this premature generalization for now. A dynamic loader can easily ensure that initialization occurs in a single goroutine, as the spec requires, and this code is not performance-critical. If a compelling need arises to make access to the guard variable atomic, then let's do that, but for now it doesn't seem necessary.
Sign in to reply to this message.
On Fri, Mar 21, 2014 at 9:54 PM, Alan Donovan <adonovan@google.com> wrote: > On 21 March 2014 18:06, Peter Collingbourne <pcc@google.com> wrote: > >> That would work for llgo, but what about the guard check? Sure, it's >> redundant for the gccgo ABI, but what if a client wants to replace it with >> some arbitrary other thing? For example, in a dynamic loading/JIT scenario, >> a client may want to use atomics to make the check thread-safe. Which is >> why I think the synthesized init function should only provide the init >> semantics for the current package and let the client decide how it should >> interact with the rest of the system. >> > > Let's call this premature generalization for now. A dynamic loader can > easily ensure that initialization occurs in a single goroutine, as the spec > requires, and this code is not performance-critical. If a compelling need > arises to make access to the guard variable atomic, then let's do that, but > for now it doesn't seem necessary. > > Making the guard variable atomic was just one other example of how a client might want to change the semantics of the init function. I don't think it should be the place of the ssa package to make these sorts of decisions for the client and have the client reverse engineer the IR that it synthesizes if it wants anything different. This approach is also prone to instability (for example, what if the ssa package one day decides to inline the called init functions?) Maybe it would be a 'one-line check' for llgo to check whether the callee is an init function. But it would probably be about as many lines for a client which needs it to add the dependent init functions to its analysis. If you still don't agree that we should be making this change, I'll drop the matter, as it's easy to work around at the moment. Peter
Sign in to reply to this message.
On 22 March 2014 01:50, Peter Collingbourne <pcc@google.com> wrote: > > Making the guard variable atomic was just one other example of how a > client might want to change the semantics of the init function. > Why would the guard variable need to be atomic? The spec requires that all initializers run in a single goroutine. > I don't think it should be the place of the ssa package to make these > sorts of decisions for the client and have the client reverse engineer the > IR that it synthesizes if it wants anything different. This approach is > also prone to instability (for example, what if the ssa package one day > decides to inline the called init functions?) > I'm sympathetic to the argument about avoiding the need for reverse engineering, but there are other ways to avoid that, e.g. I maintain a library function that removes the unwanted pkg.init() calls. This puts the complexity off to the side, rather than into the SSA builder API. > Maybe it would be a 'one-line check' for llgo to check whether the callee > is an init function. But it would probably be about as many lines for a > client which needs it to add the dependent init functions to its analysis. > The gccgo approach requires more compiler logic since the topological order of the package import graph is computed statically, not at runtime; the current modular approach is slightly simpler for clients since each one (go/ssa/interp, go/pointer, etc) need only call main.init() and main.main(). I'd be happier if the flag introduced by your CL didn't mention gccgo (since there's nothing intrinsically gcc-related about it, it's merely an alternative style)---perhaps a name like NoInitDeps? Also this CL should have a test of some kind. In any case, the tree is frozen right now in preparation for Go 1.3, so we can't commit this change until June 1. I will have a think about it in the meantime. cheers alan
Sign in to reply to this message.
On 2014/03/24 14:32:23, adonovan wrote: > On 22 March 2014 01:50, Peter Collingbourne <mailto:pcc@google.com> wrote: > > > > > > Making the guard variable atomic was just one other example of how a > > client might want to change the semantics of the init function. > > > > Why would the guard variable need to be atomic? The spec requires that all > initializers run in a single goroutine. > > > > I don't think it should be the place of the ssa package to make these > > sorts of decisions for the client and have the client reverse engineer the > > IR that it synthesizes if it wants anything different. This approach is > > also prone to instability (for example, what if the ssa package one day > > decides to inline the called init functions?) > > > > I'm sympathetic to the argument about avoiding the need for reverse > engineering, but there are other ways to avoid that, e.g. I maintain a > library function that removes the unwanted pkg.init() calls. This puts > the complexity off to the side, rather than into the SSA builder API. > > > > Maybe it would be a 'one-line check' for llgo to check whether the callee > > is an init function. But it would probably be about as many lines for a > > client which needs it to add the dependent init functions to its analysis. > > > > The gccgo approach requires more compiler logic since the topological order > of the package import graph is computed statically, not at runtime; the > current modular approach is slightly simpler for clients since each one > (go/ssa/interp, go/pointer, etc) need only call main.init() and main.main(). > > I'd be happier if the flag introduced by your CL didn't mention gccgo > (since there's nothing intrinsically gcc-related about it, it's merely an > alternative style)---perhaps a name like NoInitDeps? Also this CL should > have a test of some kind. > > In any case, the tree is frozen right now in preparation for Go 1.3, so we > can't commit this change until June 1. I will have a think about it in the > meantime. > > cheers > alan One other nice thing that I've found having bare init functions allows me to do is to teach the compiler to simulate the effect of any store instructions in the init function into the static initializers of the global variables it initializes. I've found this to substantially reduce the overhead of LLVM's alias analysis for packages like unicode, which make heavy use of global initializers. This of course also permits further optimizations. As far as I can tell, it is sound to do this so long as the init function does not use a guard variable, as with the exception of that variable, the function by construction cannot observe variables in their pre-initialization state. I'll clean up this patch and send out another version soon with your suggestions. Peter
Sign in to reply to this message.
On 27 April 2014 21:22, <pcc@google.com> wrote: > One other nice thing that I've found having bare init functions allows > me to do is to teach the compiler to simulate the effect of any store > instructions in the init function into the static initializers of the > global variables it initializes. I've found this to substantially reduce > the overhead of LLVM's alias analysis for packages like unicode, which > make heavy use of global initializers. This of course also permits > further optimizations. > Why is the global initializer expensive for your alias analysis? It's true that it's a long sequence of code that makes a bunch of stores, but I'm surprised that the topology of the resulting graph presents any significant difficulty to analysis. Package go/pointer implements Andersen's pointer analysis (which is stronger than alias analysis) and AFAICT it deals with "unicode" just fine. What do you mean by "simulate"? Just curious: you described this feature as "gccgo-style" initialization. Are you in fact using gccgo for anything, or were you merely contrasting its initializers with those of gc?
Sign in to reply to this message.
On Wed, Apr 30, 2014 at 12:53 PM, Alan Donovan <adonovan@google.com> wrote: > > On 27 April 2014 21:22, <pcc@google.com> wrote: > >> One other nice thing that I've found having bare init functions allows >> me to do is to teach the compiler to simulate the effect of any store >> instructions in the init function into the static initializers of the >> global variables it initializes. I've found this to substantially reduce >> the overhead of LLVM's alias analysis for packages like unicode, which >> make heavy use of global initializers. This of course also permits >> further optimizations. >> > > Why is the global initializer expensive for your alias analysis? It's > true that it's a long sequence of code that makes a bunch of stores, but > I'm surprised that the topology of the resulting graph presents any > significant difficulty to analysis. Package go/pointer implements > Andersen's pointer analysis (which is stronger than alias analysis) and > AFAICT it deals with "unicode" just fine. > I have to admit that I am not exactly sure why this is so expensive for LLVM's alias analysis, as I didn't investigate this too closely. All I know is that the compiler was spending a lot of time there (as observed by sampling the call graph with a debugger). It may be that I will need to look at this again later if I see other problems of this sort. But in any case, I decided to implement this because i considered it to be a strict improvement regardless of the underlying cause. What do you mean by "simulate"? > Sorry if I wasn't clear. I mean that if I visit a store instruction in the init function, and both the pointer and the value are constants and the pointer refers to a global variable which we are defining or a subcomponent thereof, instead of emitting an LLVM store instruction I update the global's initialiser. To make this more concrete, suppose that a package defines a variable a: var a struct { a, b, c int32 } = { b: 42 } When llgo visits the ssa.Global for a, it will create an LLVM global variable: %a = global { i32, i32, i32 } zeroinitializer In the init function we will encounter a store instruction that amounts to: a.b = 42 Instead of emitting an LLVM store instruction, we instead update the initialiser of %a like this: %a = global { i32, i32, i32 } { i32 0, i32 42, i32 0 } Just curious: you described this feature as "gccgo-style" initialization. > Are you in fact using gccgo for anything, or were you merely contrasting > its initializers with those of gc? > The latter. In fact, I intend to rename this flag to "BareInit" when I update the patch.
Sign in to reply to this message.
On 30 April 2014 16:27, Peter Collingbourne <pcc@google.com> wrote: > > What do you mean by "simulate"? >> > > Sorry if I wasn't clear. I mean that if I visit a store instruction in the > init function, and both the pointer and the value are constants and the > pointer refers to a global variable which we are defining or a subcomponent > thereof, instead of emitting an LLVM store instruction I update the > global's initialiser. > > To make this more concrete, suppose that a package defines a variable a: > > var a struct { a, b, c int32 } = { b: 42 } > > When llgo visits the ssa.Global for a, it will create an LLVM global > variable: > > %a = global { i32, i32, i32 } zeroinitializer > > In the init function we will encounter a store instruction that amounts to: > > a.b = 42 > > Instead of emitting an LLVM store instruction, we instead update the > initialiser of %a like this: > > %a = global { i32, i32, i32 } { i32 0, i32 42, i32 0 } > Ah, that makes sense. Perhaps go/ssa should have the concept of an initial value for Globals (which would require that Const be generalized to aggregate types) since this is what gc and gccgo do, and llgo, it would seem. I'll add it to my list. Presumably you can do this transformation for "non-bare" init function too?
Sign in to reply to this message.
On Wed, Apr 30, 2014 at 2:47 PM, Alan Donovan <adonovan@google.com> wrote: > On 30 April 2014 16:27, Peter Collingbourne <pcc@google.com> wrote: > >> >> What do you mean by "simulate"? >>> >> >> Sorry if I wasn't clear. I mean that if I visit a store instruction in >> the init function, and both the pointer and the value are constants and the >> pointer refers to a global variable which we are defining or a subcomponent >> thereof, instead of emitting an LLVM store instruction I update the >> global's initialiser. >> >> To make this more concrete, suppose that a package defines a variable a: >> >> var a struct { a, b, c int32 } = { b: 42 } >> >> When llgo visits the ssa.Global for a, it will create an LLVM global >> variable: >> >> %a = global { i32, i32, i32 } zeroinitializer >> >> In the init function we will encounter a store instruction that amounts >> to: >> >> a.b = 42 >> >> Instead of emitting an LLVM store instruction, we instead update the >> initialiser of %a like this: >> >> %a = global { i32, i32, i32 } { i32 0, i32 42, i32 0 } >> > > Ah, that makes sense. Perhaps go/ssa should have the concept of an > initial value for Globals (which would require that Const be generalized to > aggregate types) since this is what gc and gccgo do, and llgo, it would > seem. I'll add it to my list. > Yes, that would be useful. > Presumably you can do this transformation for "non-bare" init function too? > Perhaps, but we would need to be careful to exclude the guard variable, as the init function stores a value of 1 there after we load from it in the entry block. If we apply the transformation blindly, the code in the init function would never be run, as we would always be loading the initialiser of 1. Peter
Sign in to reply to this message.
On 2014/03/24 14:32:23, adonovan wrote: > On 22 March 2014 01:50, Peter Collingbourne <mailto:pcc@google.com> wrote: > > > > > > Making the guard variable atomic was just one other example of how a > > client might want to change the semantics of the init function. > > > > Why would the guard variable need to be atomic? The spec requires that all > initializers run in a single goroutine. > > > > I don't think it should be the place of the ssa package to make these > > sorts of decisions for the client and have the client reverse engineer the > > IR that it synthesizes if it wants anything different. This approach is > > also prone to instability (for example, what if the ssa package one day > > decides to inline the called init functions?) > > > > I'm sympathetic to the argument about avoiding the need for reverse > engineering, but there are other ways to avoid that, e.g. I maintain a > library function that removes the unwanted pkg.init() calls. This puts > the complexity off to the side, rather than into the SSA builder API. > > > > Maybe it would be a 'one-line check' for llgo to check whether the callee > > is an init function. But it would probably be about as many lines for a > > client which needs it to add the dependent init functions to its analysis. > > > > The gccgo approach requires more compiler logic since the topological order > of the package import graph is computed statically, not at runtime; the > current modular approach is slightly simpler for clients since each one > (go/ssa/interp, go/pointer, etc) need only call main.init() and main.main(). > > I'd be happier if the flag introduced by your CL didn't mention gccgo > (since there's nothing intrinsically gcc-related about it, it's merely an > alternative style)---perhaps a name like NoInitDeps? Also this CL should > have a test of some kind. > > In any case, the tree is frozen right now in preparation for Go 1.3, so we > can't commit this change until June 1. I will have a think about it in the > meantime. > > cheers > alan I've updated this change with your suggestions. I'll ping again after June 1.
Sign in to reply to this message.
LGTM https://codereview.appspot.com/78780043/diff/50001/cmd/ssadump/main.go File cmd/ssadump/main.go (right): https://codereview.appspot.com/78780043/diff/50001/cmd/ssadump/main.go#newcode24 cmd/ssadump/main.go:24: B build [B]are init functions: no init guards or calls to dependent inits. Perhaps I for Init? https://codereview.appspot.com/78780043/diff/50001/go/ssa/builder_test.go File go/ssa/builder_test.go (right): https://codereview.appspot.com/78780043/diff/50001/go/ssa/builder_test.go#new... go/ssa/builder_test.go:243: // Tests that synthesized init functions are correctly formed. Add a comment here motivating the feature. IIUC, something like: "The llgo compiler makes its own arrangements for initialization of dependent packages, e.g. for simpler integration with gccgo's runtime library, and to simplify the analysis whereby it deduces which stores to globals can be performed at link time."
Sign in to reply to this message.
https://codereview.appspot.com/78780043/diff/50001/cmd/ssadump/main.go File cmd/ssadump/main.go (right): https://codereview.appspot.com/78780043/diff/50001/cmd/ssadump/main.go#newcode24 cmd/ssadump/main.go:24: B build [B]are init functions: no init guards or calls to dependent inits. On 2014/06/16 16:49:55, adonovan wrote: > Perhaps I for Init? Done. https://codereview.appspot.com/78780043/diff/50001/go/ssa/builder_test.go File go/ssa/builder_test.go (right): https://codereview.appspot.com/78780043/diff/50001/go/ssa/builder_test.go#new... go/ssa/builder_test.go:243: // Tests that synthesized init functions are correctly formed. On 2014/06/16 16:49:55, adonovan wrote: > Add a comment here motivating the feature. IIUC, something like: > > "The llgo compiler makes its own arrangements for initialization of dependent > packages, e.g. for simpler integration with gccgo's runtime library, and to > simplify the analysis whereby it deduces which stores to globals can be > performed at link time." > Done.
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=961345c68af4&repo=tools *** go.tools/go/ssa: add a flag for selecting bare init functions Bare init functions omit calls to dependent init functions and the use of an init guard. They are useful in cases where the client uses a different calling convention for init functions, or cases where it is easier for a client to analyze bare init functions. LGTM=adonovan R=adonovan CC=golang-codereviews, iant https://codereview.appspot.com/78780043 Committer: Alan Donovan <adonovan@google.com>
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
