-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
Stop putting Assume
s in MIR for every enum
-as
cast
#138297
base: master
Are you sure you want to change the base?
Conversation
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
assume(const false); | ||
_2 = const 0_u16 as u16 (IntToInt); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know why people would be as
-casting uninhabited enums, but apparently it's supported 🤷
This comment has been minimized.
This comment has been minimized.
4813cfd
to
0f958c3
Compare
This comment has been minimized.
This comment has been minimized.
Now that it doesn't bloat the MIR
0f958c3
to
45d7fd2
Compare
Oh. Well, I'm glad I added that (I wish we were better about ICE follow-up tests getting a test in the tests for the place that fixed it, rather than everything just getting a UI test that the code now works.) |
The job Click to see the possible cause of the failure (guessed by this bot)
|
Now that we have
range
attributes on parameters -- so anOrdering
passed by-value is already known to be in-1, 0, +1
, for example -- these are much less useful than they used to be.Having all these extra instructions in the MIR means more instructions for MIR passes (including
check
things like borrowck) and make them look like worse inlining candidates, even though I can't think of a time it'd be bad to inline anas
cast on an enum -- after all, if the cast is allowed at all it's because reading the tag does literally nothing in LLVM.It made sense at the time, but now we have
Transmute
in MIR, and backends handle appropriateassume
ing for that, becauseas
casts can only add assumes for concrete types so any generic cases need to be handled by the backend anyway.And, TBH, I like to be able to just say "no, don't transmute your field-less enum to an integer -- the
as
cast produces the identical MIR" if anyone asks, so that they're never tempted to do the unsafe thing by hand.