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

Mir: introduce overflow operations #29769

Closed
nikomatsakis opened this issue Nov 11, 2015 · 10 comments
Closed

Mir: introduce overflow operations #29769

nikomatsakis opened this issue Nov 11, 2015 · 10 comments
Labels
A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html I-needs-decision Issue: In need of a decision. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@nikomatsakis
Copy link
Contributor

Currently the MIR doesn't handle overflow. My intended design was to add 'overflow operations' that yield a tuple (overflow-flag, result). This is roughly how LLVM does things. We would then check for overflow and panic if it occurs. @Aatch independently described this design as option 3.

@nikomatsakis nikomatsakis added I-needs-decision Issue: In need of a decision. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html labels Nov 11, 2015
@nikomatsakis
Copy link
Contributor Author

See also @eddyb's comment: #27840 (comment)

@Aatch
Copy link
Contributor

Aatch commented Mar 31, 2016

I've just looked into doing this, and have something mostly working right now. The main problem is that the codegen is much worse than the current system due to the branches and checking being more explicit now. This means that the (T, bool) has to be stored on-stack, then read from immediately.

An idea I had to try to improve this is to allow larger temporaries to have their allocas omitted as well. Part of this requires having a way to get at the fields without using an lvalue, which I have done using a new ExtractField value. This should allow it to work with immediate aggregates like those produced by the overflow intrinsics.

@nikomatsakis
Copy link
Contributor Author

@Aatch Is this ExtractField something you are proposing adding to MIR?

@eddyb
Copy link
Member

eddyb commented Mar 31, 2016

@Aatch You could generalize FatPtr to any pair of values, I think.

@Aatch
Copy link
Contributor

Aatch commented Mar 31, 2016

@nikomatsakis yes. It actually looks to be working surprisingly well. It's effectively a short-hand for Rvalue::Use(Operand::Consume(Lvalue::Projection(<field projection>))). I also disabled the code that forces an alloca for non-scalar types and everything seems to still be working, though I haven't checked that much. As far as I can tell, the comment about having to have two paths for GEP vs extractvalue is actually misleading as GEP generally corresponds to a projection, which marks a temp as requiring a alloca and therefore uses a GEP.

The codegen for overflow checking by the MIR is now basically the same as the codegen done by the current trans path, which was what my goal was. I'll have a PR ready soon I hope, so you can see what is going on.

@Aatch
Copy link
Contributor

Aatch commented Apr 4, 2016

I experimented with supporting "SSA lvalues", and it gets complicated quick. However, I think we'll want it to improve codegen around function calls, which still require an on-stack location for the return. I think I'll try to tackle that first, since it's going to be a bigger project.

@Aatch
Copy link
Contributor

Aatch commented Apr 28, 2016

Ok, so making the codegen better turns out to be surprisingly complicated. So I'm going to punt that to some future person.

Instead I've focussed on getting the rest of the checks in. The checks for x/0, x % 0, MIN / -1 and MIN % -1 are generated verbatim in the MIR as is the check for -MIN.

@eddyb
Copy link
Member

eddyb commented May 7, 2016

@Aatch I believe the cleanest solution would be to generalize FatPtr to any pair of immediates.
This also removes any boolean casting because a bool immediate is an i1.

@pnkfelix
Copy link
Member

pnkfelix commented Jun 6, 2016

This is closed by PR #33905, right?

@eddyb
Copy link
Member

eddyb commented Jun 6, 2016

Indeed. Must've missed it when I made the PR.

@eddyb eddyb closed this as completed Jun 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html I-needs-decision Issue: In need of a decision. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants