-
-
Notifications
You must be signed in to change notification settings - Fork 15.2k
match: Use an aggregate equality comparison for constant array/slice patterns when possible #155216
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
Open
jakubadamw
wants to merge
12
commits into
rust-lang:main
Choose a base branch
from
jakubadamw:issue-110870-103073
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
c56a38e
Use an aggregate equality comparison for constant array/slice pattern…
jakubadamw 61fb9cf
Add a MIR test ensuring we use an aggregate comparison when matching …
jakubadamw 78fc3d0
Add another test, a run-pass one
jakubadamw 9e1ff18
Extend both tests with the example from https://github.com/rust-lang/…
jakubadamw 99e04d7
Make sure the new aggregate equality comparison is excluded from cons…
jakubadamw fd1f06b
Update the tests to cover const contexts as well
jakubadamw 867c6bc
Raise the aggregate equality comparison threshold so simple arrays do…
jakubadamw 1d27764
Merge the AggregateEq and StringEq test arms into a single case
jakubadamw 1b24a6a
Add a missing space
jakubadamw 93c05e9
Remove the special treatment of the `const_cmp` and `const_trait_impl…
jakubadamw 8327b18
Pass the original constant value through THIR instead of reconstructi…
jakubadamw 4a8c85e
Assert that the aggregate equality comparison cannot unwind
jakubadamw File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
Oops, something went wrong.
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The general approach feels unfortunate: we transformed constants into patterns in
const_to_pat, and now we try to reverse that transformation. Have we tried keeping the original constant around in the output ofconst_to_patand using it at runtime? Tho this has the same issue of const-dependent MIR lowering that @dianne pointed out.View changes since the review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we do that, maybe we could also synthesize a constant during THIR building for hand-written array/slice patterns, like this PR currently does in MIR building? That way, we wouldn't end up worse codegen for hand-written array patterns than for const array items used as patterns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dunno, small handwritten arrays behave pretty much like tuples, it could be better codegen sometimes not to make them into constants:
It's admittedly a stretch but I'm tempted to err on the side of respecting user intent especially before the MIR boundary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, right. Maybe we should have a test for that in
mir-opt/building/match/sort_candidates.rsor such if we start optimizing array/slice comparisons? I think as-is this PR may also turn that into 8 sequential tests, each aTestKind::AggregateEqfor a different constant.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Nadrieril, as an alternative to being guided solely by user “intent”, would it be sensible to raise the threshold on the number of elements in an array pattern where we would use the aggregate equality? Right now it’s
> 2. Perhaps 4 would work better? I suppose a quantitative comparison with benchmarks could be of use here, but sadly I can’t commit to that with my present schedule.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so, yeah. I'm not really familiar with codegen yet, so I couldn't say what the best way to do that would be. Naïvely, I'm imagining adding bulk comparison to MIR; it'd contain the constant we want to bulk-compare with so that we can generate easily-optimizable LLVM IR ourselves. Unfortunately, needing to be non-panicking means we can't just model it as a call to a normal intrinsic, which means we can't provide a default implementation in
coreunless we got a panic effect so we could have non-panicking calls (..and we'd need powerful enough const generics to make the constant a generic arg, but that seems more realistic). As for how to lower it, judging by what worked for rustc 1.71.0, a straight-line sequence ofloads andicmps strung together withselects optimizes pretty well (it gets auto-vectorized), and if I understand LLVM's undef semantics, it should be correct too (as long as eachselectis switching on the result of the previous sequence of comparisons, not the new comparison). Adding to MIR looks like a pain maintenance-wise though (and potentially opsem-wise, though this should reduce to a bunch ofswitchInts so theoretically there's nothing new, I think?).. hopefully there's a lighter-weight option! Maybe I should actually try to figure out what regressed here; it'd be educational for me if nothing else.If we ever get a LIR for optimizations, maybe that could have a non-branching select in it so we could get rid of branches from these matches before passing them off to LLVM. But getting rid of branching does seem like the sort of thing LLVM should be doing... I wonder why it stopped working.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems like a lot of complexity. I'd rather change our semantics to lower "match on a constant" to an equality comparison, which is most likely what users expect anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be clear, I also don't think we should bake bulk comparisons into MIR ^^ that's way too much maintenance burden.
If we had LIR, I imagine simplifying control-flow could be useful for more than matches against constants; I don't think anything that complex should be done just for this.
I hadn't considered changing semantics. I'd want to look into what's going wrong first, but an equality comparison is probably what users expect yeah.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's a thought: I think that if we define the semantics of matching on a byte slice constant to be
PartialEq::eq, then our current lowering is a valid "optimization" over that. Does that seem right to you?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's right. Of course, if we apply it at MIR building time, as we currently do, we'd need a false unwind edge and validity assertion to make borrowck and Miri not "optimization"-dependent. Maybe it'd also need something to account for the opsem consequences of taking a reference? I'm not really familiar with those semantics.
Only having the validity assertion for byte slices/arrays seems relatively fine to me since I think that matches how
PartialEq::eqworks on slices/arrays: if it specializes to acompare_bytes/raw_eqit asserts validity for the whole slices/arrays; otherwise it compares element-by-element with short-circuiting. I'm not really sure how to feel about matches against byte slice/array constants being more restrictively borrow-checked; maybe other structural matches against named/associated constants could have false unwind edges to pretend they're more like calls toPartialEq::eq? Idk whether that's something we'd want to hold space for, though.