Skip to content

Avoid translation of unreachable arms in lazy binops #21985

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

Closed
wants to merge 1 commit into from

Conversation

dotdash
Copy link
Contributor

@dotdash dotdash commented Feb 5, 2015

No description provided.

@rust-highfive
Copy link
Contributor

r? @brson

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

@brson
Copy link
Contributor

brson commented Feb 5, 2015

Can you add a one-line comment explaining what this early return is doing?

@brson
Copy link
Contributor

brson commented Feb 5, 2015

I presume the idea here is to generate less IR since LLVM will figure this out on its own. Do you have a sense of how often this occurs in real code and what kind of savings it creates?

@dotdash
Copy link
Contributor Author

dotdash commented Feb 5, 2015

I only noticed this when looking at the unoptimized IR for libsyntax, looking for br i1 (false|true). A few dozen of those were from with_cond calls (fixed in an earlier PR), and a few hundred from from lazy binops. I don't know how representative that is or if it saves much in terms of compile time, but the places I looked at were rather hard to read due to the extra branches and basic blocks. The fact that some of them were inside matches didn't exactly help ;-)

@dotdash
Copy link
Contributor Author

dotdash commented Feb 5, 2015

Looking through the IR for libsyntax again, most of these seem to originate from derived PartialEq implementations. Maybe "fixing" those is preferable?

@dotdash
Copy link
Contributor Author

dotdash commented Feb 6, 2015

Compile times for libsyntax are unchanged. I'd be fine with dropping this and maybe fixing the deriving code at some point. In the meantime, I can easily locally apply this patch when I feel the need.

@dotdash
Copy link
Contributor Author

dotdash commented Feb 13, 2015

Not worth the added complexity

@dotdash dotdash closed this Feb 13, 2015
@dotdash dotdash deleted the binop branch February 6, 2018 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants