Implement MIR pass to lower intrinsics #41024
Conversation
Includes lowering the `move_val_init` intrinsic.
|
The tidy issue should be resolved now |
| // the cleanup code will be happily ignored and bad things will happen. | ||
|
|
||
| match func.ty.sty { | ||
| TypeVariants::TyFnDef(_, _, fn_sig) |
There was a problem hiding this comment.
this and the following line look kind of funny. Is this the correct way to format it?
There was a problem hiding this comment.
Well, I needed to break the line somewhere to avoid the 100 width limit. Maybe I could indent the if statement. I haven't seen a similar situation earlier, so I just went with what felt natural to me.
| TypeVariants::TyFnDef(_, _, fn_sig) | ||
| if fn_sig.skip_binder().abi == Abi::RustIntrinsic => { | ||
| let target = match destination { | ||
| &Some(ref d) => d.1, |
There was a problem hiding this comment.
can you do match *destnation to prevent &Some and &None?
There was a problem hiding this comment.
I don't see the added value of that
There was a problem hiding this comment.
It's the preferred style
There was a problem hiding this comment.
Ok, in that case I will change it. Thanks for pointing it out.
| args: Vec<Operand<'tcx>>, | ||
| target: BasicBlock) { | ||
| let name = &*intrinsic_name(tcx, &func); | ||
| if name == "move_val_init" { |
There was a problem hiding this comment.
will this approach encourage a long if/else if chain for every intrinsic? If so, that just seems like a nightmare to read and maintain
There was a problem hiding this comment.
I expect to introduce a match in the future and write a function for each intrinsic.
|
For reference, I do not think it is worthwhile to try land this without having at least the most prominent intrinsics ported (transmute, uninit, unreachable, needs_drop, discriminant_value, ...). Intrinsics that depend on layout like |
|
@nagisa do you think the current approach is sound? I will be happy to implement the remaining intrinsics, but it would be good to know whether I am going in the right direction. |
|
@aochagavia I think you’ll encounter some trouble trying to port most of the intrinsics with this specific approach. Some intrinsics want to be lowered later than sooner, and the design will have to account for that. |
|
I mean writing the transformation as a MIR Pass might work out, but you’ll need at least a number of these at various points. |
|
I also looked into lowering intrinsics at MIR build time, but that didn't seem very compelling either. |
|
So it looks like we need some design work before blindly lowering intrinsics. I would love to help with that, but I am no expert in Rust's internals. |
|
I have time to experiment with different possible directions, though. |
|
So if there is anything in particular that seems like it could work, feel free to tell me so I can try it out |
shepmaster
added
the
S-waiting-on-review
|
@nagisa I believe that @aochagavia is looking for a bit more guidance and direction before continuing. |
|
I’m not sure there’s anything I can tell beyond what I’ve already told here. I feel like designing suitable approach would be an equivalent, in difficulty, problem as implementing the approach. The viable design could include just having a distinct pass for every single intrinsic, but that sounds untenable in a long run. |
|
Given its weird semantics, we are supposed to handle |
|
The |
|
Based on @arielb1's last comment, it sounds like there's at least a hint of a proposed sketch of a design for intrinsics other than If there continues to be complex issues around the higher-level design, opening a thread on the internals forum might yield a deeper discussion than a PR. |
shepmaster
added
S-waiting-on-author
|
I am no longer interested in working on this. Thanks anyway for the feedback. |
Work towards #32716
The only lowering provided by this PR is for the
move_val_initintrinsic.r? @nagisa