Rodrigodd commented on issue #5316:
Pinging @jameysharp because it is the one that I talked about this.
bjorn3 commented on issue #5316:
The
func
fields needs to be mutable for cg_clif. module.declare_func_in_func() needs it as well as a hack I use to support variadic functions (https://github.com/bjorn3/rustc_codegen_cranelift/blob/a449d0d1431b789adf7c4c27e0db028ad2218a4a/src/abi/mod.rs#L542) Others may have other reasons to directly modify the function.The distinction is that even if you call func_mut(), you can only change the contents of the Function.
*builder.func_mut() = Function::new()
is allowed and replaces the entirety of the function.
jameysharp commented on issue #5316:
Oh, right, it doesn't let you change where the pointer points, but it does let you change what it points to in any way you want. I guess then that's only useful to highlight those call-sites which might break
FunctionBuilder
s invariants. Which is still worth something, but maybe not enough to justify the change.I don't understand your other comments though, @bjorn3. As far as I can tell, the callers of
Module::declare_func_in_func
and the line you pointed to incodegen_terminator_call
are all well-supported by thefunc_mut()
accessor.Regarding the
finalize()
call in cg-clif'scodegen_fn_body
: do you mind changing that to letfinalize
consume theFunctionBuilder
? It looks to me like passingbcx
as an argument instead of keeping it infx
might not be too invasive of a change, for example.
bjorn3 commented on issue #5316:
I don't understand your other comments though, @bjorn3. As far as I can tell, the callers of Module::declare_func_in_func and the line you pointed to in codegen_terminator_call are all well-supported by the func_mut() accessor.
They are. I was just saying that
func_mut()
doesn't prevent replacing the Function unlike what you said.Regarding the finalize() call in cg-clif's codegen_fn_body: do you mind changing that to let finalize consume the FunctionBuilder? It looks to me like passing bcx as an argument instead of keeping it in fx might not be too invasive of a change, for example.
Passing bcx as argument would require passing it everywhere. What would work though is to move the finalize() call to the caller of codegen_fn_body. I did have to move it out of the
tcx.sess.time("codegen clif ir")
call too which would slightly regress accuracy of performance measurements, but that isn't a big deal as I expectfinalize()
to be much cheaper than the rest of codegen_fn_body.
Rodrigodd commented on issue #5316:
fixed a warning
jameysharp commented on issue #5316:
I chatted with @cfallin to double-check my reasoning. @Rodrigodd, please drop the second commit (getters for
FunctionBuilder::func
), and I'll merge the first one (makingfinalize
consumeself
).Limiting access to the
func
field has some value, even though it doesn't prevent any bugs sincefunc_mut()
allows effectively the same thing; at least uses offunc_mut
mark where such bugs could occur. However, we concluded that's not enough to justify a breaking API change that's somewhat tedious to fix.Forcing
FunctionBuilder
to be a short-lived borrow of theFunctionBuilderContext
, on the other hand, has minimal impact on callers and clarifies how these types are intended to be used, so that change is worth doing.
jameysharp commented on issue #5316:
Thank you!
Last updated: Dec 23 2024 at 12:05 UTC