Skip to content
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

Rollup of 6 pull requests #67220

Merged
merged 15 commits into from
Dec 11, 2019
Merged

Rollup of 6 pull requests #67220

merged 15 commits into from
Dec 11, 2019

Conversation

Centril
Copy link
Contributor

@Centril Centril commented Dec 11, 2019

Successful merges:

Failed merges:

r? @ghost

Krishna Sai Veera Reddy and others added 15 commits November 29, 2019 15:22
Casting the booleans to `i8`s and converting their difference
into `Ordering` generates better assembly than casting them to
`u8`s and comparing them.
We now only propagate a scalar pair if the Rvalue is a tuple with two
scalars. This for example avoids propagating a (u8, u8) value when
Rvalue has type `((), u8, u8)` (see the regression test). While this is
a correct thing to do, implementation is tricky and will be done later.

Fixes rust-lang#66971
Fixes rust-lang#66339
Fixes rust-lang#67019
…-ord-optimization, r=sfackler

Optimize Ord trait implementation for bool

Casting the booleans to `i8`s and converting their difference into `Ordering` generates better assembly than casting them to `u8`s and comparing them.

Fixes rust-lang#66780

#### Comparison([Godbolt link](https://rust.godbolt.org/z/PjBpvF))

##### Old assembly:
```asm
example::boolean_cmp:
        mov     ecx, edi
        xor     ecx, esi
        test    esi, esi
        mov     eax, 255
        cmove   eax, ecx
        test    edi, edi
        cmovne  eax, ecx
        ret
```

##### New assembly:
```asm
example::boolean_cmp:
        mov     eax, edi
        sub     al, sil
        ret
```

##### Old LLVM-MCA statistics:
```
Iterations:        100
Instructions:      800
Total Cycles:      234
Total uOps:        1000

Dispatch Width:    6
uOps Per Cycle:    4.27
IPC:               3.42
Block RThroughput: 1.7
```

##### New LLVM-MCA statistics:
```
Iterations:        100
Instructions:      300
Total Cycles:      110
Total uOps:        500

Dispatch Width:    6
uOps Per Cycle:    4.55
IPC:               2.73
Block RThroughput: 1.0
```
Fix constant propagation for scalar pairs

We now only propagate a scalar pair if the Rvalue is a tuple with two scalars. This for example avoids propagating a (u8, u8) value when Rvalue has type `((), u8, u8)` (see the regression test). While this is a correct thing to do, implementation is tricky and will be done later.

Fixes rust-lang#66971
Fixes rust-lang#66339
Fixes rust-lang#67019
Add options to --extern flag.

This changes the `--extern` flag so that it can take a series of options that changes its behavior. The general syntax is `[opts ':'] name ['=' path]` where `opts` is a comma separated list of options. Two options are supported, `priv` which replaces `--extern-private` and `noprelude` which avoids adding the crate to the extern prelude.

```text
--extern priv:mylib=/path/to/libmylib.rlib
--extern noprelude:alloc=/path/to/liballoc.rlib
```

`noprelude` is to be used by Cargo's build-std feature in order to use `--extern` to reference standard library crates.

This also includes a second commit which adds the `aux-crate` directive to compiletest. I can split this off into a separate PR if desired, but it helps with defining these kinds of tests. It is based on rust-lang#54020, and can be used in the future to replace and simplify some of the Makefile tests.
…=oli-obk

Ensure that panicking in constants eventually errors

based on rust-lang#67134

closes rust-lang#66975

r? @oli-obk
@Centril
Copy link
Contributor Author

Centril commented Dec 11, 2019

@bors r+ p=6 rollup=never

@bors
Copy link
Contributor

bors commented Dec 11, 2019

📌 Commit f6ceef5 has been approved by Centril

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Dec 11, 2019
@Centril Centril added the rollup A PR which is a rollup label Dec 11, 2019
@Centril
Copy link
Contributor Author

Centril commented Dec 11, 2019

@bors p=2000

@bors
Copy link
Contributor

bors commented Dec 11, 2019

⌛ Testing commit f6ceef5 with merge 033662d...

bors added a commit that referenced this pull request Dec 11, 2019
Rollup of 6 pull requests

Successful merges:

 - #66881 (Optimize Ord trait implementation for bool)
 - #67015 (Fix constant propagation for scalar pairs)
 - #67074 (Add options to --extern flag.)
 - #67164 (Ensure that panicking in constants eventually errors)
 - #67174 (Remove `checked_add` in `Layout::repeat`)
 - #67205 (Make `publish_toolstate.sh` executable)

Failed merges:

r? @ghost
@bors
Copy link
Contributor

bors commented Dec 11, 2019

☀️ Test successful - checks-azure
Approved by: Centril
Pushing 033662d to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 11, 2019
@bors bors merged commit f6ceef5 into rust-lang:master Dec 11, 2019
@Centril Centril deleted the rollup-n3u9wd5 branch December 11, 2019 12:30
@bors bors mentioned this pull request Dec 11, 2019
@eddyb
Copy link
Member

eddyb commented Dec 12, 2019

Before I forget about it, I think something in this rollup caused a noticeable regression.
My initial guess was #67015, but it might not be that, according to #67015 (comment).

cc @rust-lang/wg-compiler-performance

@nikomatsakis
Copy link
Contributor

Should we file an issue to track that ?

@nnethercote
Copy link
Contributor

@oli-obk: Could #67164 have caused the regression?

@krishna-veerareddy: Could #66881 have cause the regression?

@ehuss: Could #67074 have caused the regression?

#67174 and #67205 seem very unlikely to be the cause.

I just tried to work out which commit caused the problem but failed; every revision I tried (even blatantly pre-merge ones) had the same post-regression numbers on ctfe-stress-4. Not sure what I'm doing wrong.

@krishna-veerareddy
Copy link
Contributor

@nnethercote Hey sorry for the late response. It's unlikely that 66881 could have caused a regression since it has to do with boolean comparision which isn't used quite often and if anything the fix should have reduced the instruction count but I could be wrong. Is this still an unresolved issue?

@oli-obk
Copy link
Contributor

oli-obk commented Dec 14, 2019

It could have been caused by #67164 since we now produce some code for dead code that would otherwise never have been emitted to MIR. We can likely fix this via #67191 because then we can revert the commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. rollup A PR which is a rollup S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.