make enum size not depend on the order of variants #131684
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1498,15 +1498,59 @@ pub enum TagEncoding<VariantIdx: Idx> { | ||
| Direct, | ||
|
|
||
| /// Niche (values invalid for a type) encoding the discriminant: | ||
| /// Discriminant and variant index doesn't always coincide. | ||
| /// | ||
| /// The variant `untagged_variant` contains a niche at an arbitrary | ||
| /// offset (field `tag_field` of the enum), which for a variant with | ||
| /// discriminant `d` is set to `d.wrapping_add(niche_start)`. | ||
| /// | ||
| /// As for how to compute the discriminant, we have an optimization here that we allocate discriminant | ||
| /// value starting from the variant after the `untagged_variant` when the `untagged_variant` is | ||
| /// contained in `niche_variants`' range. Thus the `untagged_variant` won't be allocated with a | ||
| /// unneeded discriminant. Motivation for this is issue #117238. | ||
| /// For example, | ||
| /// ``` | ||
| /// enum SomeEnum { | ||
| /// A, // 1 | ||
| /// B, // 2 | ||
| /// C(bool), // untagged_variant, no discriminant | ||
| /// D, // has a discriminant of 0 | ||
| /// } | ||
|
Comment on lines
+1513
to
+1518
There was a problem hiding this comment. Also, the given enum already has a size of 1. I think it's the edge-case in #117238 where we need the wraparound. If there's enough room the layout logic already shifts up the entire tag range, including a gap where the untagged variant would otherwise be. It's only when the tag range is full that that becomes a problem. Well, maybe there also are cases with range-restricted payloads other than bool. Something using But generally those should be exceptions, not the rule. So we shouldn't take a more expensive code-path for the normal case. There was a problem hiding this comment. Yeah, we can still wrap around when the tag range is full to avoid the off-by-1 bug. But taking a step back, a clippy warning might be a better approach for this rare case than complicating codegen? |
||
| /// ``` | ||
| /// The algorithm is as follows: | ||
| /// ```rust,ignore (pseudo-code) | ||
| /// // We ignore leading and trailing variants that don't need discriminants. | ||
| /// adjusted_len = niche_variants.end - niche_variants.start + 1 | ||
| /// adjusted_index = variant_index - niche_variants.start | ||
| /// d = if niche_variants.contains(untagged_variant) { | ||
| /// adjusted_untagged_index = untagged_variant - niche_variants.start | ||
| /// (adjusted_index + adjusted_len - adjusted_untagged_index) % adjusted_len - 1 | ||
| /// } else { | ||
| /// adjusted_index | ||
| /// } | ||
| /// tag_value = d.wrapping_add(niche_start) | ||
| /// ``` | ||
| /// To load variant index from tag value: | ||
| /// ```rust,ignore (pseudo-code) | ||
| /// adjusted_len = niche_variants.end - niche_variants.start + 1 | ||
| /// d = tag_value.wrapping_sub(niche_start) | ||
| /// variant_index = if niche_variants.contains(untagged_variant) { | ||
|
There was a problem hiding this comment. This code mixes compile-time and runtime behavior. I think that's unnecessarily confusing. It could be split into separate sections (one for interior untagged variants, one without) which each have their own code samples. |
||
| /// if d < adjusted_len - 1 { | ||
| /// adjusted_untagged_index = untagged_variant - niche_variants.start | ||
| /// (d + 1 + adjusted_untagged_index) % adjusted_len + niche_variants.start | ||
| /// } else { | ||
| /// // When the discriminant is larger than the number of variants having | ||
| /// // discriminant, we know it represents the untagged_variant. | ||
| /// untagged_variant | ||
| /// } | ||
| /// } else { | ||
| /// if d < adjusted_len { | ||
| /// d + niche_variants.start | ||
| /// } else { | ||
| /// untagged_variant | ||
| /// } | ||
| /// } | ||
| /// ``` | ||
| Niche { | ||
| untagged_variant: VariantIdx, | ||
| niche_variants: RangeInclusive<VariantIdx>, | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -52,10 +52,20 @@ pub(crate) fn codegen_set_discriminant<'tcx>( | ||
| variants: _, | ||
| } => { | ||
| if variant_index != untagged_variant { | ||
| let discr_len = niche_variants.end().index() - niche_variants.start().index() + 1; | ||
| let adj_idx = variant_index.index() - niche_variants.start().index(); | ||
|
|
||
| let niche = place.place_field(fx, FieldIdx::new(tag_field)); | ||
| let niche_type = fx.clif_type(niche.layout().ty).unwrap(); | ||
|
|
||
| let discr = if niche_variants.contains(&untagged_variant) { | ||
| let adj_untagged_idx = | ||
| untagged_variant.index() - niche_variants.start().index(); | ||
| (adj_idx + discr_len - adj_untagged_idx) % discr_len - 1 | ||
| } else { | ||
| adj_idx | ||
| }; | ||
| let niche_value = (discr as u128).wrapping_add(niche_start); | ||
| let niche_value = match niche_type { | ||
| types::I128 => { | ||
| let lsb = fx.bcx.ins().iconst(types::I64, niche_value as u64 as i64); | ||
| @@ -131,72 +141,91 @@ pub(crate) fn codegen_get_discriminant<'tcx>( | ||
| dest.write_cvalue(fx, res); | ||
| } | ||
| TagEncoding::Niche { untagged_variant, ref niche_variants, niche_start } => { | ||
| // See the algorithm explanation in the definition of `TagEncoding::Niche`. | ||
| let discr_len = niche_variants.end().index() - niche_variants.start().index() + 1; | ||
|
|
||
| let niche_start_value = match fx.bcx.func.dfg.value_type(tag) { | ||
| types::I128 => { | ||
| let lsb = fx.bcx.ins().iconst(types::I64, niche_start as u64 as i64); | ||
| let msb = fx.bcx.ins().iconst(types::I64, (niche_start >> 64) as u64 as i64); | ||
| fx.bcx.ins().iconcat(lsb, msb) | ||
| } | ||
| ty => fx.bcx.ins().iconst(ty, niche_start as i64), | ||
| }; | ||
|
|
||
| let (is_niche, tagged_discr) = if discr_len == 1 { | ||
| // Special case where we only have a single tagged variant. | ||
| // The untagged variant can't be contained in niche_variant's range in this case. | ||
| // Thus the discriminant of the only tagged variant is 0 and its variant index | ||
| // is the start of niche_variants. | ||
| let is_niche = codegen_icmp_imm(fx, IntCC::Equal, tag, niche_start as i128); | ||
| let tagged_discr = | ||
| fx.bcx.ins().iconst(cast_to, niche_variants.start().as_u32() as i64); | ||
| (is_niche, tagged_discr) | ||
| } else { | ||
| // General case. | ||
| let discr = fx.bcx.ins().isub(tag, niche_start_value); | ||
| let tagged_discr = clif_intcast(fx, discr, cast_to, false); | ||
| if niche_variants.contains(&untagged_variant) { | ||
| let is_niche = crate::common::codegen_icmp_imm( | ||
| fx, | ||
| IntCC::UnsignedLessThan, | ||
| discr, | ||
| (discr_len - 1) as i128, | ||
| ); | ||
| let adj_untagged_idx = | ||
| untagged_variant.index() - niche_variants.start().index(); | ||
| let untagged_delta = 1 + adj_untagged_idx; | ||
| let untagged_delta = match cast_to { | ||
| types::I128 => { | ||
| let lsb = fx.bcx.ins().iconst(types::I64, untagged_delta as i64); | ||
| let msb = fx.bcx.ins().iconst(types::I64, 0); | ||
| fx.bcx.ins().iconcat(lsb, msb) | ||
| } | ||
| ty => fx.bcx.ins().iconst(ty, untagged_delta as i64), | ||
| }; | ||
| let tagged_discr = fx.bcx.ins().iadd(tagged_discr, untagged_delta); | ||
|
|
||
| let discr_len = match cast_to { | ||
| types::I128 => { | ||
| let lsb = fx.bcx.ins().iconst(types::I64, discr_len as i64); | ||
| let msb = fx.bcx.ins().iconst(types::I64, 0); | ||
| fx.bcx.ins().iconcat(lsb, msb) | ||
| } | ||
| ty => fx.bcx.ins().iconst(ty, discr_len as i64), | ||
| }; | ||
| let tagged_discr = fx.bcx.ins().urem(tagged_discr, discr_len); | ||
|
There was a problem hiding this comment.
In the motivating issue (#117238) the full discriminant range is essentially packed 2 values for the bool, 254 values for the remaining variants. In that case we can just use wrapping arithmetic of the right type size without any remainder. So for smaller cases like the A-D case above, can we rearrange the discriminants so that they also fit the natural tag wrap-around? There was a problem hiding this comment. I didn't realize modulo operation has such bad performance. Now the foundation of this PR is mostly gone. There was a problem hiding this comment. Also from this comment it sounds like it should be possible to fix this issue without changing codegen at all. IIUC, we don't need to change anything about TagEncoding::Niche, we just need to be more clever in how we construct enums with that encoding. Or did I misunderstand? There was a problem hiding this comment. I basically implemented what that comment suggested, like allocating discriminant starting right after the untagged variant and wrapping around. This changes how we convert variant_index <--> tag, thus codegen. There was a problem hiding this comment. Codegen needs to compute the tag from the discriminant. The mapping between variant index and tag only exists during compilation and can be adjusted without codegen impact. Currently, enums with Anyway maybe @the8472 could clarify what they meant in that comment. |
||
|
|
||
| let niche_variants_start = niche_variants.start().index(); | ||
| let niche_variants_start = match cast_to { | ||
| types::I128 => { | ||
| let lsb = fx.bcx.ins().iconst(types::I64, niche_variants_start as i64); | ||
| let msb = fx.bcx.ins().iconst(types::I64, 0); | ||
| fx.bcx.ins().iconcat(lsb, msb) | ||
| } | ||
| ty => fx.bcx.ins().iconst(ty, niche_variants_start as i64), | ||
| }; | ||
| let tagged_discr = fx.bcx.ins().iadd(tagged_discr, niche_variants_start); | ||
| (is_niche, tagged_discr) | ||
| } else { | ||
| let is_niche = crate::common::codegen_icmp_imm( | ||
| fx, | ||
| IntCC::UnsignedLessThan, | ||
| discr, | ||
| (discr_len - 1) as i128, | ||
| ); | ||
| let niche_variants_start = niche_variants.start().index(); | ||
| let niche_variants_start = match cast_to { | ||
| types::I128 => { | ||
| let lsb = fx.bcx.ins().iconst(types::I64, niche_variants_start as i64); | ||
| let msb = fx.bcx.ins().iconst(types::I64, 0); | ||
bjorn3 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| fx.bcx.ins().iconcat(lsb, msb) | ||
| } | ||
| ty => fx.bcx.ins().iconst(ty, niche_variants_start as i64), | ||
| }; | ||
| let tagged_discr = fx.bcx.ins().iadd(tagged_discr, niche_variants_start); | ||
| (is_niche, tagged_discr) | ||
| } | ||
| }; | ||
|
|
||
| let untagged_variant = if cast_to == types::I128 { | ||
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.
Can you add the tag/payload values here too, not just the discriminants?