On Mon, 2023-12-11 at 09:06 -0700, Jeff Law wrote: > > > On 11/20/23 16:54, David Malcolm wrote: > > On Mon, 2023-11-20 at 16:38 -0700, Jeff Law wrote: > > > > > > > > > On 11/20/23 15:46, David Malcolm wrote: > > > > On Fri, 2023-11-17 at 14:09 -0700, Jeff Law wrote: > > > > > > > > > > > > > > > On 11/17/23 14:08, Antoni Boucher wrote: > > > > > > In contrast with the other frontends, libgccjit can be > > > > > > executed > > > > > > multiple times in a row in the same process. > > > > > Yup. I'm aware of that. Even so calling init_emit_once more > > > > > than > > > > > one > > > > > time still seems wrong. > > > > > > > > There are two approaches we follow when dealing with state > > > > stored > > > > in > > > > global variables: > > > > (a) clean it all up via the various functions called from > > > > toplev::finalize > > > > (b) make it effectively constant once initialized, with > > > > idempotent > > > > initialization > > > > > > > > The multiple in-process executions of libgccjit could pass in > > > > different > > > > code-generation options. Does the RTL-initialization logic > > > > depend > > > > anywhere on flags passed in, because if so, we're probably > > > > going to > > > > need to re-run the initialization. > > > The INIT_EXPANDERS code would be the most concerning as it's > > > implementation is totally hidden and provided by the target. I > > > wouldn't > > > be at all surprised if one or more do something evil in there. > > > That > > > probably needs to be evaluated on a target by target basis. > > > > > > The rest really do look like single init, even in a JIT > > > environment > > > kinds of things -- ie all the shared constants in RTL. > > > > I think Antoni's patch can we described as implementing "single > > init", > > in that it ensures that at least part of init_emit_once is single > > init. > > > > Is the posted patch OK by you, or do we need to rework things, and > > if > > the latter, what would be the goal? > What I'm struggling with is perhaps a problem of naming. > Conceptually > "init_emit_once" in my mind should be called once and only once. > If I > read Antoni's change correctly, we call it more than once.
I'm afraid we're already doing that, Antoni's proposed patch doesn't change that. In toplev::finalize we try to clean up as much global state as possible to allow toplev::main to be runnable again. From that point of view "once" could mean "once within an invocation of toplev::main" (if that makes it feel any less gross). > That just > feels conceptually wrong -- add to it the opaqueness of > INIT_EXPANDERS > and it feels even more wrong -- we don't know what's going on behind > the > scenes in there. Given these various concerns, I think we should go with approach (a) from above: add an emit_rtl_cc::finalizer function, and have it reset all of the globals in emit-rtl.cc. That seems like the most clear way to handle this awkward situation. Dave
