tag/niche terminology cleanup #72497

RalfJung · 2020-05-23T11:32:27Z

The term "discriminant" was used in two ways throughout the compiler:

every enum variant has a corresponding discriminant, that can be given explicitly with Variant = N.
that discriminant is then encoded in memory to store which variant is active -- but this encoded form of the discriminant was also often called "discriminant", even though it is conceptually quite different (e.g., it can be smaller in size, or even use niche-filling).

After discussion with @eddyb, this renames the second term to "tag". The way the tag is encoded can be either TagEncoding::Direct (formerly DiscriminantKind::Tag) or TagEncoding::Niche (formerly DiscrimianntKind::Niche).

This finally resolves some long-standing confusion I had about the handling of variant indices and discriminants, which surfaced in #72419.

(There is also a DiscriminantKind type in libcore, it remains unaffected. I think this corresponds to the discriminant, not the tag, so that seems all right.)

r? @eddyb

RalfJung · 2020-05-23T11:32:50Z

Cc @rust-lang/compiler

src/librustc_mir/interpret/operand.rs

eddyb · 2020-05-23T11:43:16Z

Previous relevant issue: #49938.

I think the problem here arose from trying to unify the "tagged" and "niche-filling" representations.

When we needed a word to refer to "tag or niche" we ended up with "discr(iminant)" again in some places (despite referring to the encoding of the discriminant, and not the discriminant itself), but we should've called both of them "tag" and be done with it.

RalfJung · 2020-05-23T12:03:48Z

src/librustc_codegen_llvm/debuginfo/metadata.rs

+    RegularTag { tag_field: Field, tag_type_metadata: &'ll DIType },
+    OptimizedTag,


Does this correspond to Direct and Niche encodings? If yes, would it make sense to align terminology here?

I suspected it might mean something like univariant but I think you're right. Also, instead of NoTag, maybe we could use Option around EnumTagInfo.

If we are not sure, maybe I'll just leave this as-is for now and hope someone else documents it eventually.^^

OptimizedTag only refers to Niche encodings. Direct encodings can be RegularTag or NoTag depending on whether enum fallback is happening or not. Univariant enums or structs are always NoTag.

I do like the idea of using Option<EnumTagInfo> and eliminating the NoTag variant.

r=oli-obk,eddyb with this addressed or a fixme left with instructions on what should be done

Direct encodings can be RegularTag or NoTag depending on whether enum fallback is happening or not. Univariant enums or structs are always NoTag.

What is "enum fallback"? If it's "no tag needed because there is just one variant", AFAIK those do not have a TagEncoding at all, they are Variant::Single.

@oli-obk I added a FIXME, could you check if that comment makes any sense?

What is "enum fallback"?

It's a windows thing, because windows debuginfo isn't caught up yet

bors · 2020-05-30T11:44:13Z

☔ The latest upstream changes (presumably #72768) made this pull request unmergeable. Please resolve the merge conflicts.

nikomatsakis · 2020-06-01T21:50:05Z

I think this terminology is fine but we should document it in the rustc-dev-guide

RalfJung · 2020-06-02T06:23:22Z

Where in the guide would this fit best?

nikomatsakis · 2020-06-04T14:50:57Z

@RalfJung maybe folks from @rust-lang/wg-rustc-dev-guide have thoughts, but the glossary might work :)

nikomatsakis · 2020-06-04T14:51:12Z

I feel like we should (ultimately) have a Layout section of the guide that talks about the layout code and how it works, that'd be ideal.

mark-i-m · 2020-06-04T21:48:25Z

A layout section would be awesome, but if that's not feasible, it would be great to update/add the relevant definitions to the glossary.

RalfJung · 2020-06-05T10:13:15Z

If this is accepted, I am happy to write a paragraph or two about it somewhere, if I am told where. :) The glossary makes sense. But writing an entire chapter on "layout" is a bit too much.

mark-i-m · 2020-06-05T14:33:29Z

Would it make sense to add a section to one of the "codegen" chapters?

RalfJung · 2020-06-10T08:20:53Z

So, what is this blocked on? As far as I am concerned, I am waiting for this to be accepted before I put in the work of writing docs.

nikomatsakis · 2020-06-10T10:18:50Z

My impression was that we were happy with this terminology choice. So the PR just needs review.

spastorino · 2020-06-10T19:01:54Z

Unnominating this as it was already discussed in our last T-compiler meeting.

RalfJung · 2020-06-11T09:27:25Z

I opened a dev-guide PR: rust-lang/rustc-dev-guide#747

oli-obk · 2020-06-15T11:18:31Z

r? @oli-obk

oli-obk · 2020-06-15T11:36:34Z

src/librustc_mir/interpret/operand.rs

        assert_eq!(tag_layout.size, tag_val.layout.size);
        assert_eq!(tag_layout.abi.is_signed(), tag_val.layout.abi.is_signed());
        let tag_val = tag_val.to_scalar()?;
        trace!("tag value: {:?}", tag_val);

        // Figure out which discriminant and variant this corresponds to.
-        Ok(match *tag_kind {
-            DiscriminantKind::Tag => {
+        Ok(match *tag_encoding {


I really wish we could deduplicate this with the codegen_ssa logic. Maybe we need a codegen_miri in addition to codegen_llvm. That backend could then interpret the values immediately instead of generating code. Not sure if that can be pulled off..

Anyway, off topic for this PR

oli-obk · 2020-06-16T08:50:35Z

@bors r+

bors · 2020-06-16T08:50:37Z

📌 Commit 10c8d2a has been approved by oli-obk

@eddyb

tag/niche terminology cleanup The term "discriminant" was used in two ways throughout the compiler: * every enum variant has a corresponding discriminant, that can be given explicitly with `Variant = N`. * that discriminant is then encoded in memory to store which variant is active -- but this encoded form of the discriminant was also often called "discriminant", even though it is conceptually quite different (e.g., it can be smaller in size, or even use niche-filling). After discussion with @eddyb, this renames the second term to "tag". The way the tag is encoded can be either `TagEncoding::Direct` (formerly `DiscriminantKind::Tag`) or `TagEncoding::Niche` (formerly `DiscrimianntKind::Niche`). This finally resolves some long-standing confusion I had about the handling of variant indices and discriminants, which surfaced in rust-lang#72419. (There is also a `DiscriminantKind` type in libcore, it remains unaffected. I think this corresponds to the discriminant, not the tag, so that seems all right.) r? @eddyb

@eddyb

tag/niche terminology cleanup The term "discriminant" was used in two ways throughout the compiler: * every enum variant has a corresponding discriminant, that can be given explicitly with `Variant = N`. * that discriminant is then encoded in memory to store which variant is active -- but this encoded form of the discriminant was also often called "discriminant", even though it is conceptually quite different (e.g., it can be smaller in size, or even use niche-filling). After discussion with @eddyb, this renames the second term to "tag". The way the tag is encoded can be either `TagEncoding::Direct` (formerly `DiscriminantKind::Tag`) or `TagEncoding::Niche` (formerly `DiscrimianntKind::Niche`). This finally resolves some long-standing confusion I had about the handling of variant indices and discriminants, which surfaced in rust-lang#72419. (There is also a `DiscriminantKind` type in libcore, it remains unaffected. I think this corresponds to the discriminant, not the tag, so that seems all right.) r? @eddyb

@eddyb

tag/niche terminology cleanup The term "discriminant" was used in two ways throughout the compiler: * every enum variant has a corresponding discriminant, that can be given explicitly with `Variant = N`. * that discriminant is then encoded in memory to store which variant is active -- but this encoded form of the discriminant was also often called "discriminant", even though it is conceptually quite different (e.g., it can be smaller in size, or even use niche-filling). After discussion with @eddyb, this renames the second term to "tag". The way the tag is encoded can be either `TagEncoding::Direct` (formerly `DiscriminantKind::Tag`) or `TagEncoding::Niche` (formerly `DiscrimianntKind::Niche`). This finally resolves some long-standing confusion I had about the handling of variant indices and discriminants, which surfaced in rust-lang#72419. (There is also a `DiscriminantKind` type in libcore, it remains unaffected. I think this corresponds to the discriminant, not the tag, so that seems all right.) r? @eddyb

@ghost

Rollup of 13 pull requests Successful merges: - rust-lang#70740 (Enabling static-pie for musl) - rust-lang#72331 (Report error when casting an C-like enum implementing Drop) - rust-lang#72486 (Fix asinh of negative values) - rust-lang#72497 (tag/niche terminology cleanup) - rust-lang#72999 (Create self-contained directory and move there some of external binaries/libs) - rust-lang#73130 (Remove const prop for indirects) - rust-lang#73142 (Ensure std benchmarks get tested.) - rust-lang#73305 (Disallow loading crates with non-ascii identifier name.) - rust-lang#73346 (Add rust specific features to print target features) - rust-lang#73362 (Test that bounds checks are elided when slice len is checked up-front) - rust-lang#73459 (Reduce pointer casts in Box::into_boxed_slice) - rust-lang#73464 (Document format correction) - rust-lang#73479 (Minor tweaks to liballoc) Failed merges: r? @ghost

rust-highfive assigned eddyb May 23, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 23, 2020

RalfJung commented May 23, 2020

View reviewed changes

src/librustc_mir/interpret/operand.rs Outdated Show resolved Hide resolved

RalfJung mentioned this pull request May 23, 2020

Miri read_discriminant: return a scalar instead of raw underlying bytes #72419

Merged

RalfJung force-pushed the tag-term branch 2 times, most recently from 10d7af2 to 2fa9161 Compare May 23, 2020 12:02

RalfJung commented May 23, 2020

View reviewed changes

RalfJung force-pushed the tag-term branch from 2fa9161 to 96377c9 Compare May 23, 2020 12:50

RalfJung added I-nominated T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 25, 2020

tag/niche terminology cleanup

f7d745f

RalfJung force-pushed the tag-term branch from 96377c9 to 39df54b Compare May 30, 2020 12:22

miri errors: rename InvalidDiscriminant -> InvalidTag

7a6d03c

RalfJung force-pushed the tag-term branch from 39df54b to 7a6d03c Compare May 30, 2020 12:37

spastorino removed the I-nominated label Jun 10, 2020

RalfJung mentioned this pull request Jun 11, 2020

tag/niche terminology rust-lang/rustc-dev-guide#747

Merged

rust-highfive assigned oli-obk and unassigned eddyb Jun 15, 2020

oli-obk reviewed Jun 15, 2020

View reviewed changes

add FIXME to EnumTagInfo

10c8d2a

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 16, 2020

Manishearth mentioned this pull request Jun 16, 2020

Rollup of 10 pull requests #73408

Closed

Manishearth mentioned this pull request Jun 16, 2020

Rollup of 9 pull requests #73412

Closed

Manishearth mentioned this pull request Jun 18, 2020

Rollup of 17 pull requests #73475

Closed

RalfJung mentioned this pull request Jun 19, 2020

Rollup of 13 pull requests #73498

Merged

bors merged commit 5e7eec2 into rust-lang:master Jun 19, 2020

RalfJung deleted the tag-term branch June 20, 2020 10:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tag/niche terminology cleanup #72497

tag/niche terminology cleanup #72497

RalfJung commented May 23, 2020

RalfJung commented May 23, 2020

eddyb commented May 23, 2020

RalfJung May 23, 2020

eddyb Jun 4, 2020

RalfJung Jun 5, 2020 •

edited

Loading

oli-obk Jun 15, 2020

oli-obk Jun 15, 2020

RalfJung Jun 15, 2020

RalfJung Jun 15, 2020

oli-obk Jun 16, 2020

bors commented May 30, 2020

nikomatsakis commented Jun 1, 2020

RalfJung commented Jun 2, 2020

nikomatsakis commented Jun 4, 2020

nikomatsakis commented Jun 4, 2020

mark-i-m commented Jun 4, 2020

RalfJung commented Jun 5, 2020

mark-i-m commented Jun 5, 2020

RalfJung commented Jun 10, 2020

nikomatsakis commented Jun 10, 2020

spastorino commented Jun 10, 2020 •

edited

Loading

RalfJung commented Jun 11, 2020

oli-obk commented Jun 15, 2020

oli-obk Jun 15, 2020

oli-obk commented Jun 16, 2020

bors commented Jun 16, 2020

		RegularTag { tag_field: Field, tag_type_metadata: &'ll DIType },
		OptimizedTag,

tag/niche terminology cleanup #72497

tag/niche terminology cleanup #72497

Conversation

RalfJung commented May 23, 2020

RalfJung commented May 23, 2020

eddyb commented May 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Jun 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented May 30, 2020

nikomatsakis commented Jun 1, 2020

RalfJung commented Jun 2, 2020

nikomatsakis commented Jun 4, 2020

nikomatsakis commented Jun 4, 2020

mark-i-m commented Jun 4, 2020

RalfJung commented Jun 5, 2020

mark-i-m commented Jun 5, 2020

RalfJung commented Jun 10, 2020

nikomatsakis commented Jun 10, 2020

spastorino commented Jun 10, 2020 • edited Loading

RalfJung commented Jun 11, 2020

oli-obk commented Jun 15, 2020

Choose a reason for hiding this comment

oli-obk commented Jun 16, 2020

bors commented Jun 16, 2020

RalfJung Jun 5, 2020 •

edited

Loading

spastorino commented Jun 10, 2020 •

edited

Loading