Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cleanup: Share DST size/align computation logic between codegen backends (and interpreter) #103728

Open
RalfJung opened this issue Oct 29, 2022 · 3 comments
Labels
A-codegen Area: Code generation A-const-eval Area: constant evaluation (mir interpretation) A-cranelift Things relevant to the [future] cranelift backend C-cleanup Category: PRs that clean code up or issues documenting cleanup.

Comments

@RalfJung
Copy link
Member

Currently, each codegen backend and the MIR interpreter needs to re-implement this logic:

pub fn size_and_align_of_dst<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(

pub(crate) fn size_and_align_of_dst<'tcx>(

pub(super) fn size_and_align_of(

Turns out the interpreter version actually has a bug. Ideally we'd only have this logic once so that we don't need the same subtle logic in multiple places.

Cc @bjorn3 @eddyb

@RalfJung RalfJung added the C-cleanup Category: PRs that clean code up or issues documenting cleanup. label Oct 29, 2022
@bjorn3 bjorn3 added A-codegen Area: Code generation A-const-eval Area: constant evaluation (mir interpretation) A-cranelift Things relevant to the [future] cranelift backend labels Oct 29, 2022
@eddyb
Copy link
Member

eddyb commented Nov 1, 2022

I think the problem is that this is one of those hypothetical "MIR->LIR reusable lowering steps" that IIRC I discussed with @oli-obk before (just like e.g. the codegen for the MIR casts that involve CoerceUnsize and bottom out in "*T/&T built-in unsizing"), that require monomorphic types to fully resolve and we just don't have such a "LIR" concept yet (the closest thing being the builder API of rustc_codegen_ssa).

Maybe if it can be expressed as a post-monomorphization MIR->MIR "simplification" we can ignore the lack of a "LIR"? It would have to be at the level of e.g. statements, and on the fly (like monomorphization itself), not a whole-body pass, if we want to avoid large amounts of wasteful allocations (tho I may be wrong about how much that matters, heh).

@RalfJung
Copy link
Member Author

RalfJung commented Nov 1, 2022

Alternatively we could do something specific, like a generic function for the logic that invokes callback to do the arithmetic on whatever domain the backend needs (values for Miri, LLVM/cranelift expressions for codegen).

@oli-obk
Copy link
Contributor

oli-obk commented Nov 1, 2022

It would have to be at the level of e.g. statements, and on the fly (like monomorphization itself), not a whole-body pass, if we want to avoid large amounts of wasteful allocations (tho I may be wrong about how much that matters, heh).

We should probably just try out monomorphizing and optimizing all MIR bodies before codegen processes it and see how bad it is. So far we've only said "probably bad" and never actually collected any numbers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area: Code generation A-const-eval Area: constant evaluation (mir interpretation) A-cranelift Things relevant to the [future] cranelift backend C-cleanup Category: PRs that clean code up or issues documenting cleanup.
Projects
None yet
Development

No branches or pull requests

4 participants