Skip to content

Refactor object file handling - #70384

Merged
bors merged 3 commits into
rust-lang:masterfrom
nnethercote:refactor-object-file-handling
Mar 27, 2020
Merged

Refactor object file handling#70384
bors merged 3 commits into
rust-lang:masterfrom
nnethercote:refactor-object-file-handling

Conversation

@nnethercote

@nnethercotennethercote commented Mar 25, 2020

Copy link
Copy Markdown
Contributor

Some preliminary clean-ups that grease the path to #66961.

r? @alexcrichton

@nnethercote
Remove TargetOptions::embed_bitcode.
e1d1db7
It's unused by any existing targets, and soon we'll be embedding full
bitcode by default anyway.
@rust-highfiverust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 25, 2020
@nnethercote
nnethercote removed the request for review from alexcrichtonMarch 25, 2020 06:17
@nnethercote
nnethercoteforce-pushed the refactor-object-file-handling branch from 7d2aa8c to 72f1c6cCompareMarch 25, 2020 06:21
@nnethercote

nnethercote commented Mar 25, 2020

Copy link
Copy Markdown
ContributorAuthor

In theory this shouldn't affect performance at all, but let's check that:

@bors try @rust-timer queue

@rust-timer

rust-timer commented Mar 25, 2020

Copy link
Copy Markdown
Collaborator

Awaiting bors try build completion

@bors

bors commented Mar 25, 2020

Copy link
Copy Markdown
Collaborator

⌛ Trying commit 72f1c6cfed177a356c7e05d16ba778cb1cb45078 with merge 8753044243364ce6751612dea9eae4439dcad8ce...

@michaelwoerister

michaelwoerister commented Mar 25, 2020

Copy link
Copy Markdown
Member

If I remember correctly "marker" bitcode embedding had something to do with iOS compatibility? #35968 has some discussion on it; but @alexcrichton would know more about that anyway.

@bors

bors commented Mar 25, 2020

Copy link
Copy Markdown
Collaborator

☀️ Try build successful - checks-azure
Build commit: 8753044243364ce6751612dea9eae4439dcad8ce (8753044243364ce6751612dea9eae4439dcad8ce)

@rust-timer

rust-timer commented Mar 25, 2020

Copy link
Copy Markdown
Collaborator

Queued 8753044243364ce6751612dea9eae4439dcad8ce with parent cdb50c6, future comparison URL.

alexcrichton
alexcrichton reviewed Mar 25, 2020
View reviewed changes

@alexcrichtonalexcrichton left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm always a fan of cleanups :)

Comment threadsrc/librustc_codegen_llvm/back/write.rs Outdated
Comment threadsrc/librustc_codegen_llvm/back/write.rs Outdated
Comment threadsrc/librustc_codegen_llvm/back/write.rs Outdated
Comment threadsrc/librustc_codegen_ssa/back/write.rs Outdated
@nnethercote

nnethercote commented Mar 26, 2020

Copy link
Copy Markdown
ContributorAuthor

The perf impact is negligible, as expected.

@nnethercote
Introduce EmitObj.
87ef16c
Currently, there are three fields in `ModuleConfig` that dictate
how object files are emitted: `emit_obj`, `obj_is_bitcode`, and
`embed_bitcode`.
Some of the combinations of these fields are nonsensical, in particular
having both `obj_is_bitcode` and `embed_bitcode` true at the same time.
Also, currently:
- we needlessly emit and then delete a bytecode file if `obj_is_bitcode`
is true but `emit_obj` is false;
- we needlessly embed bitcode in the LLVM module if `embed_bitcode` is
true and `emit_obj` is false.
This commit combines the three fields into one, with a new type
`EmitObj` (and the auxiliary `BitcodeSection`) which can encode five
different possibilities.
In the old code, `set_flags` would set `obj_is_bitcode` and
`embed_bitcode` on all three of the configs (`modules`, `allocator`,
`metadata`) if the relevant other conditions were met, even if no object
code needed to be emitted for one or more of them. Whereas
`start_async_codegen` would set `emit_obj`, but only for those configs
that need it.
In the new code, `start_async_codegen` does all the work of setting
`emit_obj`, and it only does that for the configs that need it.
`set_flags` no longer sets anything related to object file emission.
@nnethercote
Convert a chained if-else to a match.
a50cca9
It makes things a little clearer.
@nnethercote
nnethercoteforce-pushed the refactor-object-file-handling branch from 72f1c6c to a50cca9CompareMarch 26, 2020 03:14
@nnethercote

nnethercote commented Mar 26, 2020

Copy link
Copy Markdown
ContributorAuthor

@alexcrichton New code is up. I reinstated marker bitcode sections (in a nicer fashion than we had before) and did other minor changes you suggested where possible.

@alexcrichton

alexcrichton commented Mar 26, 2020

Copy link
Copy Markdown
Member

@bors: r+

@bors

bors commented Mar 26, 2020

Copy link
Copy Markdown
Collaborator

📌 Commit a50cca9 has been approved by alexcrichton

@borsbors 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 Mar 26, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Mar 26, 2020
@Dylan-DPC
Rollup merge ofrust-lang#70384- nnethercote:refactor-object-file-ha…
c916a78
…ndling, r=alexcrichton
Refactor object file handling
Some preliminary clean-ups that grease the path to rust-lang#66961.
r? @alexcrichton
@Dylan-DPC-zzDylan-DPC-zz mentioned this pull request Mar 26, 2020
Closed
@Dylan-DPC-zzDylan-DPC-zz mentioned this pull request Mar 26, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 26, 2020
@bors
Auto merge ofrust-lang#70441- Dylan-DPC:rollup-qv7h2ph, r=Dylan-DPC
62c6006
Rollup of 6 pull requests
Successful merges:
- rust-lang#70384 (Refactor object file handling)
- rust-lang#70397 (add 'fn write_u16s' to Memory)
- rust-lang#70413 (Fix incorrect pattern warning "unreachable pattern")
- rust-lang#70428 (`error_bad_item_kind`: add help text)
- rust-lang#70429 (Clean up E0459 explanation)
- rust-lang#70437 (Miri float->int casts: be explicit that this is saturating)
Failed merges:
r? @ghost
@bors
bors merged commit b15423e into rust-lang:masterMar 27, 2020
@nnethercote
nnethercote deleted the refactor-object-file-handling branch March 27, 2020 00:33
@nnethercotennethercote mentioned this pull request Mar 27, 2020
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alexcrichtonalexcrichtonalexcrichton approved these changes

Assignees

@alexcrichtonalexcrichton

Labels

S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@nnethercote@rust-timer@bors@michaelwoerister@alexcrichton@rust-highfive