Use discriminant_value intrinsic for derive(PartialOrd) #24270

pnkfelix · 2015-04-10T14:21:01Z

Use discriminant_value intrinsic for derive(PartialOrd)

[breaking-change]

This is a [breaking-change] because it can change the result of comparison operators when enum discriminants have been explicitly assigned. Notably in a case like:

#[derive(PartialOrd)]
enum E { A = 2, B = 1}

Under the old deriving, A < B held, because A came before B in the order of declaration. But now we use the ordering according to the provided values, and thus A > B. (However, this change is very unlikely to break much, if any, code, since the orderings themselves should all remain well-defined, total, etc.)

Fix #15523

Implements an intrinsic for extracting the value of the discriminant enum variant values. For non-enum types, this returns zero, otherwise it returns the value we use for discriminant comparisons. This means that enum types that do not have a discriminant will also work in this arrangement. This is (at least part of) the work on Issue rust-lang#24263

Fix rust-lang#15523.

rust-highfive · 2015-04-10T14:21:13Z

r? @brson

(rust_highfive has picked a reviewer for you, use r? to override)

pnkfelix · 2015-04-10T14:21:14Z

r? @huonw

pnkfelix · 2015-04-10T17:09:29Z

src/libsyntax/ext/deriving/generic/mod.rs

+ rules: ast::UnsafeBlock(ast::CompilerGenerated),
+ span: sp }));
+
+ // FIXME: This unconditionally casts to `isize`. However:


oops this FIXME is now out-of-date. Will fix and push -f.

…rrors. remove out of date fixme.

…ior commit.

sfackler · 2015-04-10T17:27:37Z

src/libsyntax/ext/deriving/generic/mod.rs

- ast::TupleVariantKind(..) => ast::PatEnum(path, None),
- ast::StructVariantKind(..) => ast::PatStruct(path, Vec::new(), true),
- })
+fn find_repr_type_name(diagnostic: &SpanHandler,


It seems like there's a chance that this could miss a repr attribute that is applied by a later pass of expansion.

Very interesting, I'm not even sure how we can handle that ... without some somewhat serious revisions to the expansion infrastructure (e.g. some way to schedule an expansion "at the end" ...)

The most conservative solution would be to change the intrinsic to return (sign, abs-value) to avoid having to figure out the repr at all, but that does make the comparisons a bit less efficient depending on how smart LLVM can be.

There is a simpler option. The intrinsic could be:

compare_variant_order<T:Reflect>(t: &T, u: &T)

which would yield a -1,0,1.

(well, we may still want the discriminant_value intrinsic itself independently, but I am inclined to agree that this is an elegant way to resolve all of the issues here.)

Ah, yeah, that's a good call.

brson · 2015-04-10T17:34:15Z

@bors r+ p=1

Though I agree about @sfackler's point I'm assuming it's not a show-stopper for the entire feature, and don't think it's going to be fixed right now.

bors · 2015-04-10T17:34:16Z

📌 Commit 847a897 has been approved by brson

bors · 2015-04-10T19:17:41Z

⌛ Testing commit 847a897 with merge c35ffe7...

bors · 2015-04-10T19:25:13Z

⛄ The build was interrupted to prioritize another pull request.

bors · 2015-04-10T19:25:15Z

⌛ Testing commit 847a897 with merge 0877916...

Use `discriminant_value` intrinsic for `derive(PartialOrd)` [breaking-change] This is a [breaking-change] because it can change the result of comparison operators when enum discriminants have been explicitly assigned. Notably in a case like: ```rust #[derive(PartialOrd)] enum E { A = 2, B = 1} ``` Under the old deriving, `A < B` held, because `A` came before `B` in the order of declaration. But now we use the ordering according to the provided values, and thus `A > B`. (However, this change is very unlikely to break much, if any, code, since the orderings themselves should all remain well-defined, total, etc.) Fix #15523

bors · 2015-04-10T20:52:36Z

💔 Test failed - auto-mac-32-opt

…insic.

pnkfelix · 2015-04-10T22:51:36Z

@bors r=brson 0877916

bors · 2015-04-10T22:51:38Z

🙀 0877916 is not a valid commit SHA. Please try again with 05aaad1.

pnkfelix · 2015-04-10T22:52:23Z

@bors r=brson 05aaad1 p=1

Use `discriminant_value` intrinsic for `derive(PartialOrd)` [breaking-change] This is a [breaking-change] because it can change the result of comparison operators when enum discriminants have been explicitly assigned. Notably in a case like: ```rust #[derive(PartialOrd)] enum E { A = 2, B = 1} ``` Under the old deriving, `A < B` held, because `A` came before `B` in the order of declaration. But now we use the ordering according to the provided values, and thus `A > B`. (However, this change is very unlikely to break much, if any, code, since the orderings themselves should all remain well-defined, total, etc.) Fix #15523

bors · 2015-04-10T23:49:25Z

⌛ Testing commit 05aaad1 with merge 93f7fe3...

bors · 2015-04-11T01:59:16Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-32-nopt-t, auto-win-32-opt, auto-win-64-nopt-t, auto-win-64-opt

pnkfelix · 2015-04-11T07:23:03Z

Filed #24320 as a follow-on issue to @sfackler 's note about the fragility of the lookup here.

@steveklabnik

r? @steveklabnik Should land this, #24270, and #24245 before rolling another beta.

ranma42 · 2015-09-01T09:06:45Z

Mentioning #24263 so that the two issues link to each other.
Does this close #13623, #15620 and #20856 ?

dotdash · 2015-09-01T09:13:34Z

No, at least the PartialEq issues are still a problem

ranma42 · 2015-09-01T10:36:07Z

I am unable to reproduce issues with the quality of optimised LLVM IR code. The --pretty expanded output can still be very redundant, both for PartialEq and PartialOrd. In particular it looks like it would be possible to remove the nullary enum variants from the match. This might also help with reducing the automatically-generated intermediate code for huge C enums, which might be good to reduce the load on the LLVM optimiser (although hopefully we don't spend much time compiling derived code anyway).
If there is any interest in this, I should be able to write a PR for it.

dotdash · 2015-09-01T11:36:03Z

Consider this:

#[derive(PartialEq)]
enum E {
  A(i32),
  B(i32),
  C(i32),
  D(i32),
}

Ideally, eq would just do a single compare (on 64bit) or two compares (on 32bit). But instead we get this:

_ZN23E...std..cmp..PartialEq2eq20hd7e6941040691683taaE:
    .cfi_startproc
    movl    (%rdi), %eax
    cmpl    (%rsi), %eax
    jne .LBB0_1
    cmpl    $1, %eax
    je  .LBB0_5
    cmpl    $2, %eax
    je  .LBB0_5
    cmpl    $3, %eax
.LBB0_5:
    movl    4(%rdi), %eax
    cmpl    4(%rsi), %eax
    sete    %al
    retq
.LBB0_1:
    xorl    %eax, %eax
    retq

And if you add a few more variants, we end up with a jump table.

ranma42 · 2015-09-01T12:11:10Z

AFAICT none of the issues I mentioned is about this kind of problem.

It looks like you're suggesting that we should try to unify the code paths of all of the variants that share the same signature (or possibly even just the same signature prefix?). This is a (strong) generalisation of what I was suggesting, as nullary signatures are always matching. Several additional complications some to my mind, though, as this seems to interact with the memory layout of the enum (is there any guarantee that the i32 values from your example are always stored in the same position?).
Also, what about different types which might have the same size?

#[derive(PartialEq)]
enum E {
  A(i32),
  B(isize),
}

Should the Rust compiler be able to unify the comparison assuming that the on the target arch isize is 32 bits?

I believe that such optimisations might be more appropriate in LLVM than in the Rust compiler. As a matter of fact, although the LLVM IR code keeps them duplicated, upon translation to assembly LLVM is already able to unify the comparisons of the four cases. It is pretty weird that it cannot optimise the jump table correctly. Apparently it gets rid of the last je (which would be pointing to the next instruction), but then it does not realise that the result of cmpl $3, %eax is unused and so on.
Maybe we could ask for advice to the LLVM developers.

Should a new issue be filed to track it or an existing one be updated to make this specific problem more prominent?

dotdash · 2015-09-01T12:36:09Z

#13623 was meant to be about all enums, not just C-like ones. I'll add the above example to it to clarify that.

That LLVM should do most optimizations is right, but in some cases we should also generate code that is easier for LLVM to optimize. Also, while the final asm has the code unified, the IR is still large, which means more work for the optimizer and possibly missed opportunities for inlining.

This is the same approach taken in rust-lang#24270, except that this should not be a breaking change because it only changes the output of hash functions, which nobody should be relying on.

Aatch and others added 4 commits April 10, 2015 12:23

Add test for discriminant_value results

800c5f8

Rebase discriminant_value test. Add case for a specialized repr.

ea27391

Change derive expansions to use discriminant_value intrinsic.

6118795

Fix rust-lang#15523.

rust-highfive assigned brson Apr 10, 2015

rust-highfive assigned huonw and unassigned brson Apr 10, 2015

pnkfelix added 2 commits April 10, 2015 16:32

Test case for new derive(PartialOrd) expansion.

c44d40e

Re-add a fixme after some investigation into what's going on.

afb7acf

pnkfelix force-pushed the use-disr-val-for-derive-ord branch from 3519318 to afb7acf Compare April 10, 2015 14:32

pnkfelix reviewed Apr 10, 2015
View reviewed changes

pnkfelix added 2 commits April 10, 2015 19:11

Incorporate repr-attr into deriving(PartialOrd) to avoid truncation e…

781fc90

…rrors. remove out of date fixme.

Test case for 64-bit corner cases where truncation occurred before pr…

47016f9

…ior commit.

pnkfelix force-pushed the use-disr-val-for-derive-ord branch from e9dbfae to 47016f9 Compare April 10, 2015 17:11

fix some comments.

847a897

brson mentioned this pull request Apr 10, 2015

Bump prerelease version #24287

Merged

pnkfelix mentioned this pull request Apr 10, 2015

default representation for enum discriminant should be i32 #24290

Closed

sfackler reviewed Apr 10, 2015
View reviewed changes

Remove pretty-expanded from 2 tests; deriving(Ord) uses unstable intr…

05aaad1

…insic.

bors merged commit 05aaad1 into rust-lang:master Apr 11, 2015

pnkfelix mentioned this pull request Apr 11, 2015

Remove repr-lookup attempt in derive expansion #24320

Closed

bors added a commit that referenced this pull request Apr 14, 2015

Auto merge of #24287 - brson:beta, r=steveklabnik

5dc8060

r? @steveklabnik Should land this, #24270, and #24245 before rolling another beta.

apasel422 mentioned this pull request Feb 28, 2016

derive(Hash) on enum causes hash collision on 32-bit system #21714

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use discriminant_value intrinsic for derive(PartialOrd) #24270

Use discriminant_value intrinsic for derive(PartialOrd) #24270

pnkfelix commented Apr 10, 2015

rust-highfive commented Apr 10, 2015

pnkfelix commented Apr 10, 2015

pnkfelix Apr 10, 2015

sfackler Apr 10, 2015

pnkfelix Apr 10, 2015

sfackler Apr 10, 2015

nikomatsakis Apr 10, 2015

pnkfelix Apr 10, 2015

sfackler Apr 10, 2015

brson commented Apr 10, 2015

bors commented Apr 10, 2015

bors commented Apr 10, 2015

bors commented Apr 10, 2015

bors commented Apr 10, 2015

bors commented Apr 10, 2015

pnkfelix commented Apr 10, 2015

bors commented Apr 10, 2015

pnkfelix commented Apr 10, 2015

bors commented Apr 10, 2015

bors commented Apr 11, 2015

pnkfelix commented Apr 11, 2015

ranma42 commented Sep 1, 2015

dotdash commented Sep 1, 2015

ranma42 commented Sep 1, 2015

dotdash commented Sep 1, 2015

ranma42 commented Sep 1, 2015

dotdash commented Sep 1, 2015

Use discriminant_value intrinsic for derive(PartialOrd) #24270

Use discriminant_value intrinsic for derive(PartialOrd) #24270

Conversation

pnkfelix commented Apr 10, 2015

rust-highfive commented Apr 10, 2015

pnkfelix commented Apr 10, 2015

pnkfelix Apr 10, 2015

Choose a reason for hiding this comment

sfackler Apr 10, 2015

Choose a reason for hiding this comment

pnkfelix Apr 10, 2015

Choose a reason for hiding this comment

sfackler Apr 10, 2015

Choose a reason for hiding this comment

nikomatsakis Apr 10, 2015

Choose a reason for hiding this comment

pnkfelix Apr 10, 2015

Choose a reason for hiding this comment

sfackler Apr 10, 2015

Choose a reason for hiding this comment

brson commented Apr 10, 2015

bors commented Apr 10, 2015

bors commented Apr 10, 2015

bors commented Apr 10, 2015

bors commented Apr 10, 2015

bors commented Apr 10, 2015

pnkfelix commented Apr 10, 2015

bors commented Apr 10, 2015

pnkfelix commented Apr 10, 2015

bors commented Apr 10, 2015

bors commented Apr 11, 2015

pnkfelix commented Apr 11, 2015

ranma42 commented Sep 1, 2015

dotdash commented Sep 1, 2015

ranma42 commented Sep 1, 2015

dotdash commented Sep 1, 2015

ranma42 commented Sep 1, 2015

dotdash commented Sep 1, 2015