Generate valid_ranges for enums sign-agnostically#159509
Conversation
|
Some changes occurred in compiler/rustc_hir/src/attrs cc @jdonszelmann, @JonathanBrouwer Some changes occurred in compiler/rustc_attr_parsing |
This comment has been minimized.
This comment has been minimized.
89681fc to
fc374f1
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment has been minimized.
This comment has been minimized.
fc374f1 to
f8b4450
Compare
|
Ok, the first version I'd misunderstood how we need to pick tag signedness, so redid that and added a bunch of new tests because there were disturbingly few failures when I'd probably broken a ton of stuff (albeit subtly). OP updated as well to describe the fixed approach. |
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Generate `valid_range`s for enums sign-agnostically
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (8d18ada): comparison URL. Overall result: no relevant changes - no action neededBenchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up. @rustbot label: -S-waiting-on-perf -perf-regression Instruction countThis perf run didn't have relevant results for this metric. Max RSS (memory usage)This perf run didn't have relevant results for this metric. CyclesResults (primary -2.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 489.126s -> 483.723s (-1.10%) |
Prompted by #159438 (comment), this reworks the generation of the tag layout information for enums.
Having gone down the wrong path at first here, I think the core trouble this is having is that the current calculation is trying to serve two incompatible desires:
value: Primitive, we need to respect the signedness of the original discriminant numbers as typed in Rust.valid_range: WrappingRange, we want to ignore the signedness (and size!) of the discriminants and look only at the bit values (and size) of the tags themselves.This thus splits the handling in
layout_of_enuminto those two parts, handled differently.discr_range_of_repr), we obey the signedness of the inputs to find the smallest negative discriminant and largest positive discriminant -- notably without any wraparound logic.WrappingRangeto represent those tags (which can be a different range than we'd have picked on the discriminants, especially forrepr(Rust)where the discriminants areisize).With apologies for sending this when you'd said you'd look at it, but I got inspired 😅
r? @oli-obk
Fixes #159438
I have a bunch of commits in here; it's probably best reviewed one at a time.
WrappingRangeto its own file, since thelib.rsis over 2000 lines already so if I was going to add stuff I figured some reorganizing could help.WrappingRangethat takes an iterator of values and returns a range containing those values. Dealing just inu128+Size, likeWrappingRangedoes in other places, it doesn't even have signedness information so structurally is prevented from having any "why arerepr(u8)and `repr(i8) picking different ranges?" issues. I found it helpful to have this split out from the layout code, for example that it let me put some examples on the doc-comment. Nothing actually uses the new function in this commit though.i128since https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/util/struct.Discr.html#structfield.val points out that they're generated asu128in the same way thatWrappingRangeuses already -- even with the field doc that-1_i8is generated as255_u128, so theas i128ing was probably just making everything more confusing here than it needed to be.Primitiveandvalid_range, along with rustc support for#[rustc_dump_layout(largest_niche)]to help have these tests include specific ERROR annotations rather than just the stderr. The tests pass at this point, so the next commit can update them to show the change.WrappingRangeconstructor added earlier.