add an `Rvalue` for `is_val_statically_known` and use it in MIR optimization #146550

joboet · 2025-09-14T18:35:06Z

By lowering the is_val_statically_known to an Rvalue instead of passing it through to codegen, it becomes easier to optimise in the MIR-opt const propagation passes.

rustbot · 2025-09-14T18:35:09Z

⚠️ #[rustc_intrinsic_const_stable_indirect] controls whether intrinsics can be exposed to stable const
code; adding it needs t-lang approval.

cc @rust-lang/wg-const-eval

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

This PR changes rustc_public

cc @oli-obk, @celinval, @ouz-a

Some changes occurred to constck

cc @fee1-dead

This PR changes MIR

cc @oli-obk, @RalfJung, @JakobDegen, @vakaras

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter
gets adapted for the changes, if necessary.

cc @rust-lang/miri, @RalfJung, @oli-obk, @lcnr

rustbot · 2025-09-14T18:35:10Z

r? @SparrowLii

rustbot has assigned @SparrowLii.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

joboet · 2025-09-14T18:36:21Z

compiler/rustc_public/src/unstable/convert/stable/mir.rs

            }
            CopyForDeref(place) => crate::mir::Rvalue::CopyForDeref(place.stable(tables, cx)),
            WrapUnsafeBinder(..) => todo!("FIXME(unsafe_binders):"),
+            StaticallyKnown(..) => todo!(),


Can I just add this to the public API or is there a special procedure for that?

RalfJung · 2025-09-14T20:01:12Z

No fundamental objection, but I am slightly concerned that if we have to make every intrinsic that optimizations should recognize into a MIR primitive, things will get unwieldy.

It's not that hard to check if a given function call invokes a particular intrinsic (and we can add more convenience operations to make that simpler). What's the problem with using that approach?

oli-obk · 2025-09-15T10:28:54Z

I am slightly concerned that if we have to make every intrinsic that optimizations should recognize into a MIR primitive, things will get unwieldy.

It also avoids requiring a terminator and new block, so for some intrinsics, changing them to an rvalue is very useful. I have not investigated whether that is true here, but it was one of the original motivations for lowering some intrinsics to rvalues

…ization

rustbot · 2025-09-15T10:43:41Z

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

oli-obk · 2025-09-15T11:06:58Z

Ok gave the changes a review.

It's a lot of extra stuff to handle for sth that only one specific kind of optimization cares about. Does gvn so far have no optimizations that act on terminators, possibly replacing them wholesale?

joboet · 2025-09-15T11:09:00Z

No fundamental objection, but I am slightly concerned that if we have to make every intrinsic that optimizations should recognize into a MIR primitive, things will get unwieldy.

It's not that hard to check if a given function call invokes a particular intrinsic (and we can add more convenience operations to make that simpler). What's the problem with using that approach?

It also avoids requiring a terminator and new block, so for some intrinsics, changing them to an rvalue is very useful. I have not investigated whether that is true here, but it was one of the original motivations for lowering some intrinsics to rvalues

That's one of the reasons here – is_val_statically_known is mostly used in conjunction with things like comparisons, so knowing that it does not have side-effects could be very helpful, as it might allow optimizations to simplify is_val_statically_known() && /* some comparison */ to is_val_statically_known & /* some comparison */, which might simplify the CFG a lot.

Also, this just felt right to me: is_val_statically_known is a very fundamental and one-of-a-kind primitive in the optimisation pipeline as it one of the only observables of optimization.

But at the end of the day, this is my second compiler PR and I mostly made this decision just to follow the established pattern, so I don't feel too strongly.

joboet · 2025-09-15T11:11:36Z

Ok gave the changes a review.

It's a lot of extra stuff to handle for sth that only one specific kind of optimization cares about. Does gvn so far have no optimizations that act on terminators, possibly replacing them wholesale?

Not really, but I guess it wouldn't be too difficult:

rust/compiler/rustc_mir_transform/src/gvn.rs

Lines 1732 to 1753 in d1ed52b

    
               fn visit_terminator(&mut self, terminator: &mut Terminator<'tcx>, location: Location) { 
        
                   if let Terminator { kind: TerminatorKind::Call { destination, .. }, .. } = terminator { 
        
                       if let Some(local) = destination.as_local() 
        
                           && self.ssa.is_ssa(local) 
        
                       { 
        
                           let ty = self.local_decls[local].ty; 
        
                           let opaque = self.new_opaque(ty); 
        
                           self.assign(local, opaque); 
        
                       } 
        
                   } 
        
                   // Function calls and ASM may invalidate (nested) derefs. We must handle them carefully. 
        
                   // Currently, only preserving derefs for trivial terminators like SwitchInt and Goto. 
        
                   let safe_to_preserve_derefs = matches!( 
        
                       terminator.kind, 
        
                       TerminatorKind::SwitchInt { .. } | TerminatorKind::Goto { .. } 
        
                   ); 
        
                   if !safe_to_preserve_derefs { 
        
                       self.invalidate_derefs(); 
        
                   } 
        
                   self.super_terminator(terminator, location); 
        
               } 
        
           }

rust-log-analyzer · 2025-09-15T11:15:17Z

The job pr-check-2 failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)

[RUSTC-TIMING] declare_clippy_lint test:false 0.092
    Checking unicode-width v0.1.14
[RUSTC-TIMING] cargo_metadata test:false 0.777
    Checking quine-mc_cluskey v0.2.4
error[E0004]: non-exhaustive patterns: `&rustc_middle::mir::Rvalue::StaticallyKnown(_)` not covered
    --> src/tools/clippy/clippy_utils/src/qualify_min_const_fn.rs:127:11
     |
 127 |     match rvalue {
     |           ^^^^^^ pattern `&rustc_middle::mir::Rvalue::StaticallyKnown(_)` not covered
     |
note: `rustc_middle::mir::Rvalue<'_>` defined here
    --> /checkout/compiler/rustc_middle/src/mir/syntax.rs:1368:1
     |
1368 | pub enum Rvalue<'tcx> {
     | ^^^^^^^^^^^^^^^^^^^^^
...
1498 |     StaticallyKnown(Operand<'tcx>),
     |     --------------- not covered
     = note: the matched value is of type `&rustc_middle::mir::Rvalue<'_>`
help: ensure that all possible cases are being handled by adding a match arm with a wildcard pattern or an explicit pattern as shown
     |
 214 ~         },
 215 ~         &rustc_middle::mir::Rvalue::StaticallyKnown(_) => todo!(),
     |

[RUSTC-TIMING] toml_edit test:false 1.399
    Checking toml v0.7.8
[RUSTC-TIMING] unicode_width test:false 0.158

bjorn3 · 2025-09-15T11:43:28Z

compiler/rustc_middle/src/mir/syntax.rs

+    ///
+    /// From an operational semantics perspective, the result is
+    /// non-deterministically chosen.
+    StaticallyKnown(Operand<'tcx>),


I assume this will still evaluate the Operand for UB side-effects?

Yes, at least it might. Skipping it should be sound though, right? And miri should detect the UB because const-eval calls eval_operand...

RalfJung · 2025-09-15T12:22:41Z

library/core/src/intrinsics/mod.rs

-pub const fn is_val_statically_known<T: Copy>(_arg: T) -> bool {
-    false
-}
+#[rustc_intrinsic_const_stable_indirect]


Adding this attribute should not be needed for the rvalue refactor. Please don't just do a const-stabilization as a drive-by change! It's not even mentioned in the PR description.

As the comment at the top of this file explains, this needs t-lang FCP.

Oh wait, there was a rustc_const_stable_indirect before. Things just got reordered making this harder to see.

It's probably silly to keep an unused fallback body just to avoid this attribute. I assume t-lang won't insist on an FCP but we should still ping them when we resolved the other discussions in this PR.

RalfJung · 2025-09-15T12:26:28Z

src/tools/miri/src/intrinsics/mod.rs

-                // FIXME: should we check for validity here? It's tricky because we do not have a
-                // place. Codegen does not seem to set any attributes like `noundef` for intrinsic
-                // calls, so we don't *have* to do anything.


This comment is still relevant. Please be careful not to remove critical comments like this.

bors · 2025-09-17T12:08:10Z

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

rustbot assigned SparrowLii Sep 14, 2025

joboet commented Sep 14, 2025

View reviewed changes

This comment has been minimized.

Sign in to view

add an Rvalue for is_val_statically_known and use it in MIR optim…

bbcf44f

…ization

joboet force-pushed the is_val_statically_known_rvalue branch from 751ecb9 to bbcf44f Compare September 15, 2025 10:43

bjorn3 reviewed Sep 15, 2025

View reviewed changes

RalfJung reviewed Sep 15, 2025

View reviewed changes

add an Rvalue for is_val_statically_known and use it in MIR optimization #146550

Are you sure you want to change the base?

add an Rvalue for is_val_statically_known and use it in MIR optimization #146550

Uh oh!

Conversation

joboet commented Sep 14, 2025

Uh oh!

rustbot commented Sep 14, 2025

Uh oh!

rustbot commented Sep 14, 2025

Uh oh!

joboet Sep 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

RalfJung commented Sep 14, 2025

Uh oh!

oli-obk commented Sep 15, 2025

Uh oh!

rustbot commented Sep 15, 2025

Uh oh!

oli-obk commented Sep 15, 2025

Uh oh!

joboet commented Sep 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joboet commented Sep 15, 2025

Uh oh!

rust-log-analyzer commented Sep 15, 2025

Uh oh!

bjorn3 Sep 15, 2025

Choose a reason for hiding this comment

Uh oh!

joboet Sep 15, 2025

Choose a reason for hiding this comment

Uh oh!

RalfJung Sep 15, 2025

Choose a reason for hiding this comment

Uh oh!

RalfJung Sep 15, 2025

Choose a reason for hiding this comment

Uh oh!

RalfJung Sep 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bors commented Sep 17, 2025

Uh oh!

Uh oh!

add an `Rvalue` for `is_val_statically_known` and use it in MIR optimization #146550

add an `Rvalue` for `is_val_statically_known` and use it in MIR optimization #146550

joboet Sep 14, 2025 •

edited

Loading

joboet commented Sep 15, 2025 •

edited

Loading

RalfJung Sep 15, 2025 •

edited

Loading