StableMIR: Implement CompilerInterface
#139852
Conversation
rustbot
added
S-waiting-on-review
|
r? @celinval btw should we create a tracking issue for this? |
| @@ -233,7 +234,12 @@ where | |||
| mir_consts: IndexMap::default(), | |||
| layouts: IndexMap::default(), | |||
| })); | |||
| stable_mir::compiler_interface::run(&tables, || init(&tables, f)) | |||
|
|
|||
| let interface = CompilerInterface { cx: tables }; | |||
There was a problem hiding this comment.
I put the interface here since I'd prefer not to refactor the launch process now. It would be more appropriate to do this at the end.
| /// Stable-MIR’s interface to compiler internals. | ||
| /// All internal queries must go through [`Context`]. | ||
| /// | ||
| /// Do not use this directly. |
There was a problem hiding this comment.
Can you please document that this is currently used in the macro expansion?
There was a problem hiding this comment.
hmmm tbh it is not. We directly create a CompilerInterface in the rustc_internal::run() function for now, which is also called by pretty::write_smir_pretty(), not only the run_driver!.
There was a problem hiding this comment.
but yeah, maybe we can make it into a macro
There was a problem hiding this comment.
Oh my bad! I got it wrong...
| // A thread local variable that stores a pointer to the tables mapping between TyCtxt | ||
| // datastructures and stable MIR datastructures | ||
| /// Stable-MIR’s interface to compiler internals. | ||
| /// All internal queries must go through [`Context`]. |
There was a problem hiding this comment.
Can we remove Context in this PR?
Also, I think it may be worth documenting this a bit better, so people know why this is here. Something on the lines that this structure will serve as a bridge between StableMIR and the rustc_smir which will provide similar APIs but based on internal rustc constructs.
There was a problem hiding this comment.
Can we remove
Contextin this PR?
Sorry that i didn't get it. You mean we directly write something like impl<'tcx> TablesWrapper<'tcx>, then move everything that used to be in Context to there?
There was a problem hiding this comment.
Oh and then we create a brand-new Context in rustc_smir, and gradually move methods from TablesWrapper to it until TablesWrapper is completely hollowed out.
There was a problem hiding this comment.
Yes, you don't need the trait anymore. Next step I believe would be to move the stable / internal calls out of the TablesWrapper.
Btw, you could rename TablesWrapper to Context too.
There was a problem hiding this comment.
Also, feel free to improve the name scheme, it's not my strongest skill. LoL
There was a problem hiding this comment.
hmmm what if we change CompilerInterface to SmirInterface?🤔
| scoped_tls::scoped_thread_local!(static TLV: Cell<*const ()>); | ||
|
|
||
| pub fn run<F, T>(context: &dyn Context, f: F) -> Result<T, Error> | ||
| pub fn run<'tcx, T, F>(interface: &CompilerInterface<'tcx>, f: F) -> Result<T, Error> |
There was a problem hiding this comment.
@oli-obk Any chance you can take a look at this TLV magic? I could use a second pair of eyes here. Thanks!
| pub(crate) struct Context<'tcx>(pub RefCell<Tables<'tcx>>); | ||
|
|
||
| impl<'tcx> Context<'tcx> { | ||
| pub(crate) fn target_info(&self) -> MachineInfo { |
There was a problem hiding this comment.
You might as well make these public so we don't have to update the visibility when we pull the StableMir stuff back to its crate
- With `Context` wrapped by `SmirInterface`, the stable-mir's TLV stores a pointer to `SmirInterface`, while the rustc-specific TLV stores a pointer to tables. - This PR make the `rustc_smir` mod public.
| /// similar APIs but based on internal rustc constructs. | ||
| /// | ||
| /// Do not use this directly. This is currently used in the macro expansion. | ||
| pub struct SmirInterface<'tcx> { |
There was a problem hiding this comment.
On the other hand, this one should be pub(crate), since users of stable_mir should not interact with it.
| use stable_mir::ty::IndexedVal; | ||
|
|
||
| use crate::rustc_smir::context::TablesWrapper; | ||
| use crate::rustc_smir::context::Context; |
There was a problem hiding this comment.
I wonder if we should call this SmirContext
This PR implements part of the document.
With
TablesWrapperwrapped byCompilerInterface, the stable-mir's TLV stores a pointer toCompilerInterface, while the rustc-specific TLV stores a pointer to tables.