Store scalar pair bools as i8 in memory by cuviper · Pull Request #51583 · rust-lang/rust

cuviper · 2018-06-15T23:08:38Z

We represent bool as i1 in a ScalarPair, unlike other aggregates,
to optimize IR for checked operators and the like. With this patch, we
still do so when the pair is an immediate value, but we use the i8
memory type when the value is loaded or stored as an LLVM aggregate.

So (bool, bool) looks like an { i1, i1 } immediate, but { i8, i8 }
in memory. When a pair is a direct function argument, PassMode::Pair,
it is still passed using the immediate i1 type, but as a return value
it will use the i8 memory type. Also, bool-like` enum tags will now
use scalar pairs when possible, where they were previously excluded due
to optimization issues.

Fixes #51516.
Closes #51566.

r? @eddyb
cc @nox

eddyb · 2018-06-16T06:25:38Z

{ i1, i1 } immediate

Note that we keep the two components separate and outside a LLVM aggregate when immediate, so that type should not exist.

eddyb · 2018-06-16T06:28:30Z

You can just remove this check and the comment above it.

eddyb · 2018-06-16T06:29:56Z

The type {i1, i1} is never created, so maybe say "two i1s" instead?

eddyb · 2018-06-16T06:36:06Z

Oh, this is another unfortunate consequence, I forgot about return values. I wonder if they should be using the pair "passing mode" that arguments do, and create a LLVM aggregate type on the fly, using the immediate types for the pair components. That way we'd get {i1, i1} for returns, but everything else would see {i8, i8}.

Not sure it's worth the complexity though. When inlining, LLVM should collapse the zext followed by trunc, just like it gets rid of packing into a pair and unpacking it.

eddyb · 2018-06-16T06:38:30Z

Would be cool to see this with branches, since they only ever take i1.

TimNN · 2018-07-03T17:10:07Z

Ping from triage, @cuviper: It looks like there are some comments on your PR, do you think you'll be able to address those?

cuviper · 2018-07-03T17:16:03Z

Sorry I forgot about this. Yes, I'll update for those comments.

We represent `bool` as `i1` in a `ScalarPair`, unlike other aggregates, to optimize IR for checked operators and the like. With this patch, we still do so when the pair is an immediate value, but we use the `i8` memory type when the value is loaded or stored as an LLVM aggregate. So `(bool, bool)` looks like an `{ i1, i1 }` immediate, but `{ i8, i8 }` in memory. When a pair is a direct function argument, `PassMode::Pair`, it is still passed using the immediate `i1` type, but as a return value it will use the `i8` memory type. Also, `bool`-like` enum tags will now use scalar pairs when possible, where they were previously excluded due to optimization issues.

cuviper · 2018-07-05T21:25:38Z

@eddyb - I've addressed your review comments, except for what you said about return values. Would you like a FIXME comment and/or an issue filed about that?

emilyalbini · 2018-07-09T11:24:19Z

Ping from triage @eddyb! This PR needs your review.

eddyb · 2018-07-09T12:25:28Z

@cuviper r=me with maybe an issue filed about that, but I wouldn't worry too much. Thanks!

cuviper · 2018-07-10T01:20:59Z

OK, filed #52198. Not sure if I have bors access, but let's try:

@bors r=eddyb

bors · 2018-07-10T01:21:00Z

@cuviper: 🔑 Insufficient privileges: Not in reviewers

Mark-Simulacrum · 2018-07-10T01:23:21Z

@bors r=eddyb delegate+

bors · 2018-07-10T01:23:22Z

✌️ @cuviper can now approve this pull request

bors · 2018-07-10T01:23:23Z

📌 Commit 557736b has been approved by eddyb

Mark-Simulacrum · 2018-07-10T01:36:55Z

@bors r+ p=5 (rollup fairness)

bors · 2018-07-10T01:36:56Z

💡 This pull request was already approved, no need to approve it again.

There's another pull request that is currently being tested, blocking this pull request: Haiku: work around the lack of setrlimit #52100

bors · 2018-07-10T01:36:56Z

📌 Commit 557736b has been approved by Mark-Simulacrum

bors · 2018-07-10T03:08:53Z

⌛ Testing commit 557736b with merge b3e7d70...

@eddyb

Store scalar pair bools as i8 in memory We represent `bool` as `i1` in a `ScalarPair`, unlike other aggregates, to optimize IR for checked operators and the like. With this patch, we still do so when the pair is an immediate value, but we use the `i8` memory type when the value is loaded or stored as an LLVM aggregate. So `(bool, bool)` looks like an `{ i1, i1 }` immediate, but `{ i8, i8 }` in memory. When a pair is a direct function argument, `PassMode::Pair`, it is still passed using the immediate `i1` type, but as a return value it will use the `i8` memory type. Also, `bool`-like` enum tags will now use scalar pairs when possible, where they were previously excluded due to optimization issues. Fixes #51516. Closes #51566. r? @eddyb cc @nox

bors · 2018-07-10T05:14:50Z

☀️ Test successful - status-appveyor, status-travis
Approved by: Mark-Simulacrum
Pushing b3e7d70 to master...

rust-highfive assigned eddyb Jun 15, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 15, 2018

eddyb suggested changes Jun 16, 2018

View reviewed changes

eddyb reviewed Jun 16, 2018

View reviewed changes

emilyalbini mentioned this pull request Jun 25, 2018

Store bools as i8 in scalar pairs #51566

Closed

emilyalbini added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 25, 2018

cuviper added 2 commits July 5, 2018 09:59

Update scalar pairs per review comments

557736b

cuviper force-pushed the packed_pair-bool branch from 08cfb58 to 557736b Compare July 5, 2018 21:23

cuviper added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 5, 2018

cuviper mentioned this pull request Jul 10, 2018

Consider using pair mode to return scalar pair bools as i1 #52198

Open

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 Jul 10, 2018

bors merged commit 557736b into rust-lang:master Jul 10, 2018

bors mentioned this pull request Jul 10, 2018

Make librustc_codegen_llvm aware of LLVM address spaces. #51576

Closed

cuviper deleted the packed_pair-bool branch March 27, 2019 22:21

workingjubilee mentioned this pull request Feb 10, 2023

std::fs::create_dir not failing on existing directory on aarch64, debug #50516

Closed

Uh oh!

Conversation

cuviper commented Jun 15, 2018

Uh oh!

eddyb commented Jun 16, 2018

Uh oh!

eddyb Jun 16, 2018

Choose a reason for hiding this comment

Uh oh!

eddyb Jun 16, 2018

Choose a reason for hiding this comment

Uh oh!

eddyb Jun 16, 2018

Choose a reason for hiding this comment

Uh oh!

eddyb Jun 16, 2018

Choose a reason for hiding this comment

Uh oh!

TimNN commented Jul 3, 2018

Uh oh!

cuviper commented Jul 3, 2018

Uh oh!

cuviper commented Jul 5, 2018

Uh oh!

emilyalbini commented Jul 9, 2018

Uh oh!

eddyb commented Jul 9, 2018

Uh oh!

cuviper commented Jul 10, 2018

Uh oh!

bors commented Jul 10, 2018

Uh oh!

Mark-Simulacrum commented Jul 10, 2018

Uh oh!

bors commented Jul 10, 2018

Uh oh!

bors commented Jul 10, 2018

Uh oh!

Mark-Simulacrum commented Jul 10, 2018

Uh oh!

bors commented Jul 10, 2018

Uh oh!

bors commented Jul 10, 2018

Uh oh!

bors commented Jul 10, 2018

Uh oh!

bors commented Jul 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants