Cleanup codegen_llvm/back #54864
Merged
Cleanup codegen_llvm/back #54864
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @pnkfelix (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
pnkfelix
reviewed
Oct 8, 2018
|
@pnkfelix: you mean the whitespace one? No; those lines were hand-picked after having done all the other changes. I adjusted those spots along the way. |
|
It seems @pnkfelix is busy with NLL; r? @Mark-Simulacrum |
ljedrz
force-pushed
the
cleanup_codegen_llvm_back
branch
from
a544455
to
60e123d
Compare
|
@Mark-Simulacrum thanks for the thorough review! I think I addressed all the issues. |
ljedrz
force-pushed
the
cleanup_codegen_llvm_back
branch
from
60e123d
to
ec65cff
Compare
|
@Mark-Simulacrum r? I won't be able to rebase for the next few days. |
sinkuu
reviewed
Oct 19, 2018
|
The deletion was an accident. Travis should still be happy. |
ljedrz
force-pushed
the
cleanup_codegen_llvm_back
branch
from
ec65cff
to
d2f2446
Compare
|
Included the suggestion by @sinkuu. |
|
☔ The latest upstream changes (presumably #55349) made this pull request unmergeable. Please resolve the merge conflicts. |
ljedrz
force-pushed
the
cleanup_codegen_llvm_back
branch
from
d2f2446
to
b74c9d6
Compare
|
Rebased. |
Mark-Simulacrum
approved these changes
Nov 5, 2018
|
@bors r+ |
|
📌 Commit b74c9d60bf23b7d975dc87f5efeadd5fe937f605 has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit b74c9d60bf23b7d975dc87f5efeadd5fe937f605 with merge f0097bd6148de603f9f56d906f5a286d568be711... |
bors
added
S-waiting-on-review
ljedrz
force-pushed
the
cleanup_codegen_llvm_back
branch
from
50c7e80
to
2f99d09
Compare
|
I removed the memory-related commit; maybe I will come up with a solution in the future, but there's no point in holding back the rest of the changes. |
|
@bors r=Mark-Simulacrum |
|
📌 Commit 2f99d09 has been approved by |
bors
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Nov 10, 2018
…acrum Cleanup codegen_llvm/back - improve allocations - use `Cow<'static, str>` where applicable - use `to_owned` instead of `to_string` with string literals - remove a redundant `continue` - possible minor speedup in logic - use `mem::replace` instead of `swap` where more concise - remove `'static` from consts - improve common patterns - remove explicit `return`s - whitespace & formatting fixes
|
☀️ Test successful - status-appveyor, status-travis |
bors
added a commit
that referenced
this pull request
Nov 11, 2018
codegen_llvm_back: improve allocations This commit was split out from #54864. Last time it was causing an LLVM OOM, presumably due to aggressive preallocation strategy in `thin_lto`. This time preallocations are more cautious and there are a few additional memory-related improvements (last 3 points from the list below). - _gently_ preallocate vectors of known length - `extend` instead of `append` where the argument is consumable - turn 2 `push` loops into `extend`s - create a vector from a function producing one instead of using `extend_from_slice` on it - consume `modules` when no longer needed - return an `impl Iterator` from `generate_lto_work` - don't `collect` `globals`, as they are iterated over and consumed right afterwards While I'm hoping it won't cause an OOM anymore, I would still consider this a "high-risk" PR and not roll it up.
bors
added a commit
that referenced
this pull request
Dec 4, 2018
codegen_llvm_back: improve allocations This commit was split out from #54864. Last time it was causing an LLVM OOM, which was most probably caused by not collecting the globals. - preallocate vectors of known length - `extend` instead of `append` where the argument is consumable - turn 2 `push` loops into `extend`s - create a vector from a function producing one instead of using `extend_from_slice` on it - consume `modules` when no longer needed - ~~return an `impl Iterator` from `generate_lto_work`~~ - ~~don't `collect` `globals`, as they are iterated over and consumed right afterwards~~ While I'm hoping it won't cause an OOM anymore, I would still consider this a "high-risk" PR and not roll it up.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Cow<'static, str>where applicableto_ownedinstead ofto_stringwith string literalscontinuemem::replaceinstead ofswapwhere more concise'staticfrom constsreturns