Skip to content
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

Rework and optimize match MIR generation #136123

Draft
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

xacrimon
Copy link

@xacrimon xacrimon commented Jan 27, 2025

left to do:

  • cleanup
  • code comments
  • tcx arena strategy
  • const eq
  • handle non u8 types
  • less pricky about prefixes
  • also handle slice midsections and suffixes
  • probably more?
  • tidy
  • ci
  • tests
  • perf improvements?
  • handle type changing automatic deref/ref properly
  • const eq handling for `user_ty constants in patterns
  • more generic const subsequence handling involving bindings or wildcard patterns
  • inspect generated mir
  • phf? trie? generate an efficient decision tree to decide between patterns?
  • and patterns and other things?
  • subslicing/indexing bounds & check them so we don't UB
  • look at earlier prs?
  • reduce code size?
  • handle indirect constant patterns
  • inspect the mir, are we doing more work than needed? can we generate match trees in a better way? sorting?
  • fix duplicated slice eq arms
  • tests?

example code:

use std::{num::NonZero, ffi::CStr};
const STANDARD_BASE: &[u8] = b"org.freedesktop.DBus.Error.";
#[no_mangle]
pub(crate) fn to_errno(s: &CStr) -> Option<NonZero<i32>> {
    let (pfx, sfx) = s.to_bytes().split_at_checked(STANDARD_BASE.len())?;
    if pfx != STANDARD_BASE {
        return None;
    }
    match sfx {
        b"NoMemory" => NonZero::new(1),
        b"ServiceUnknown" | b"UnknownObject" | b"UnknownInterface" | b"UnknownMethod"
        | b"UnknownProperty" | b"NameHasNoOwner" => NonZero::new(18),
        b"AccessDenied" | b"AuthFailed" | b"InteractiveAuthorizationRequired" => {
            NonZero::new(1245)
        }
        b"LimitsExceeded" => NonZero::new(9),
        b"Timeout" | b"TimedOut" => NonZero::new(420),
        b"UnixProcessIdUnknown" => NonZero::new(69),
        b"NotSupported" => NonZero::new(228),
        b"IOError" | b"NoNetwork" | b"AddressInUse" => NonZero::new(100500),
        _ => None,
    }
}

duplicate arms example

#[no_mangle]
pub(crate) fn has_magic(s: &[u8]) -> bool {
    matches!(
        s,
        [b'm', b'a', b'g', b'i', b'c', .., b'e', b'n', b'd']
            | [b's', b't', b'a', b'r', b't', .., b'e', b'n', b'd']
    )
}

without this patch: mir and aarch64asm,opt-level=3
with this patch: mir and aarch64asm,opt-level=3

note to self: should probably be looked at by compiler-errors and scottmcm, oli?

@rustbot
Copy link
Collaborator

rustbot commented Jan 27, 2025

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @compiler-errors (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 27, 2025
@xacrimon
Copy link
Author

r? @ghost

@rustbot
Copy link
Collaborator

rustbot commented Jan 27, 2025

Failed to set assignee to ghost: invalid assignee

Note: Only org members with at least the repository "read" role, users with write permissions, or people who have commented on the PR may be assigned.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@xacrimon xacrimon force-pushed the joel/bytestr-match-2 branch from af28115 to 68cb418 Compare January 27, 2025 15:59
@rust-log-analyzer

This comment has been minimized.

@rustbot rustbot added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Jan 28, 2025
@rust-log-analyzer

This comment has been minimized.

@xacrimon xacrimon changed the title Optimize constant slice patterns + byte strings Rework and optimize match MIR generation Jan 28, 2025
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Jan 29, 2025

☔ The latest upstream changes (presumably #136227) made this pull request unmergeable. Please resolve the merge conflicts.

@xacrimon
Copy link
Author

@rustbot label +S-waiting-on-author -S-waiting-on-review

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 30, 2025
@xacrimon xacrimon force-pushed the joel/bytestr-match-2 branch from c0adb36 to 18f18c5 Compare January 30, 2025 12:56
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-tools failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
   Compiling rustc_codegen_llvm v0.0.0 (/checkout/compiler/rustc_codegen_llvm)
error[E0023]: this pattern has 2 fields, but the corresponding tuple variant has 1 field
   --> compiler/rustc_mir_build/src/builder/matches/match_pair.rs:170:41
    |
170 |             && let ty::ConstKind::Value(_, valtree) = const_.kind()
    |                                         ^  ^^^^^^^ expected 1 field, found 2
   ::: /checkout/compiler/rustc_type_ir/src/const_kind.rs:34:11
    |
34  |     Value(I::ValueConst),
    |           ------------- tuple variant has 1 field
    |           ------------- tuple variant has 1 field

error[E0023]: this pattern has 2 fields, but the corresponding tuple variant has 1 field
   --> compiler/rustc_mir_build/src/builder/matches/match_pair.rs:182:41
    |
182 |             && let ty::ConstKind::Value(_, valtree) = const_.kind()
    |                                         ^  ^^^^^^^ expected 1 field, found 2
   ::: /checkout/compiler/rustc_type_ir/src/const_kind.rs:34:11
    |
34  |     Value(I::ValueConst),
    |           ------------- tuple variant has 1 field
    |           ------------- tuple variant has 1 field

error[E0061]: this enum variant takes 1 argument but 2 arguments were supplied
   --> compiler/rustc_mir_build/src/builder/matches/match_pair.rs:214:44
    |
214 |         let ty_const = ty::Const::new(tcx, ty::ConstKind::Value(const_ty, valtree));
    |                                            ^^^^^^^^^^^^^^^^^^^^ --------  ------- unexpected argument #2 of type `ValTree<'tcx>`
    |                                                                 expected `Value<'_>`, found `Ty<'_>`
    |
note: tuple variant defined here
   --> /checkout/compiler/rustc_type_ir/src/const_kind.rs:34:5
   --> /checkout/compiler/rustc_type_ir/src/const_kind.rs:34:5
    |
34  |     Value(I::ValueConst),
    |     ^^^^^
help: remove the extra argument
    |
214 -         let ty_const = ty::Const::new(tcx, ty::ConstKind::Value(const_ty, valtree));
214 +         let ty_const = ty::Const::new(tcx, ty::ConstKind::Value(/* rustc_middle::ty::Value<'_> */));

Some errors have detailed explanations: E0023, E0061.
For more information about an error, try `rustc --explain E0023`.
error: could not compile `rustc_mir_build` (lib) due to 3 previous errors

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants