Skip to content

Conversation

nnethercote
Copy link
Contributor

Various improvements to integral pattern matching. Together they reduce instruction counts for unicode_normalization-check-clean by about 16%.

r? @Mark-Simulacrum

This reduces the number of call sites for
`constructor_intersects_pattern()` from two to one, which the next
commit will take advantage of.
This is a 2% instruction count win on `unicode_normalization-check-clean`.
This commit moves a lot of code around but doesn't change functionality
at all. The main goal was for `from_pat()` to no longer call
`from_ctor()`.

The increase in inlining resulted in less function call overhead, for a
3% instruction count win on `unicode_normalization-check-clean`.
The `if let Some(val) = value.try_eval_bits(...)` branch in `from_const()` is
very hot for the `unicode_normalization` benchmark.

This commit introduces a special-case alternative for scalars that avoids
`try_eval_bits()` and all the functions it calls (`Const::eval()`,
`ConstValue::try_to_bits()`, `ConstValue::try_to_scalar()`, and
`Scalar::to_bits()`), instead extracting the result immediately.

The type and value checking done by `Scalar::to_bits()` is replicated by moving
it into a new function `Scalar::check_raw()` and using that new function in the
special case.

PR rust-lang#64673 introduced some special-case handling of scalar types in
`Const::try_eval_bits()`. This handling is now moved out of that function into
the new `IntRange::integral_size_and_signed_bias` function.

This commit reduces the instruction count for
`unicode_normalization-check-clean` by about 10%.
`filter_map()` is less general, but more efficient, and has the same
effect in this case.

This commit reduces the instruction count for
`unicode_normalization-check-clean` by about 2%.
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 4, 2019
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum left a comment

Choose a reason for hiding this comment

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

Changes look good to me; they're all just shuffling code around so we can probably land this without additional review, but cc @oli-obk

@Centril
Copy link
Contributor

Centril commented Oct 4, 2019

r? @varkor

@Centril
Copy link
Contributor

Centril commented Oct 4, 2019

oops; r? @Mark-Simulacrum but cc @varkor :)

@Mark-Simulacrum
Copy link
Member

@bors r+ rollup=never since perf

@bors
Copy link
Collaborator

bors commented Oct 5, 2019

📌 Commit 2a3a544 has been approved by Mark-Simulacrum

@bors 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 Oct 5, 2019
@bors
Copy link
Collaborator

bors commented Oct 6, 2019

⌛ Testing commit 2a3a544 with merge 9203ee7...

bors added a commit that referenced this pull request Oct 6, 2019
…, r=Mark-Simulacrum

Optimize integral pattern matching

Various improvements to integral pattern matching. Together they reduce instruction counts for `unicode_normalization-check-clean` by about 16%.

r? @Mark-Simulacrum
@bors
Copy link
Collaborator

bors commented Oct 6, 2019

☀️ Test successful - checks-azure
Approved by: Mark-Simulacrum
Pushing 9203ee7 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 6, 2019
@bors bors merged commit 2a3a544 into rust-lang:master Oct 6, 2019
@nnethercote nnethercote deleted the optimize-integral-pattern-matching branch October 6, 2019 21:26
Integer::from_attr(&tcx, UnsignedInt(uty)).size()
}
_ => tcx.layout_of(param_env.with_reveal_all().and(ty)).ok()?.size,
};
Copy link
Member

@RalfJung RalfJung Oct 12, 2019

Choose a reason for hiding this comment

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

A quick grep shows a few more sites that call layout_of just to get the size or alignment (and there's likely more but split across multiple lines). So it might still be worth having size_and_align_of(Ty<'tcx>) somewhere with an optimization like this -- maybe?

Copy link
Contributor

Choose a reason for hiding this comment

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

yea, this could just be a method on TyCtxt

Nadrieril added a commit to Nadrieril/rust that referenced this pull request Oct 18, 2019
This reverses changes from rust-lang#65089, but I need this temporarily
for rebase to be tractable.
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. 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.

9 participants