See through aggregates in GVN

[cjgillot]

This PR is extracted from #111344

The first 2 commit are cleanups to avoid repeated work. I propose to stop removing useless assignments as part of this pass, and let a later SimplifyLocals do it. This makes tests easier to read (among others).

The next 3 commits add a constant folding mechanism to the GVN pass, presented in #116012. This pass is designed to only use global allocations, to avoid any risk of accidental modification of the stored state.

The following commits implement opportunistic simplifications, in particular:

• projections of aggregates: MyStruct { x: a }.x gets replaced by a, works with enums too;
• projections of arrays: [a, b][0] becomes a;
• projections of repeat expressions: [a; N][x] becomes a;
• transform arrays of equal operands into a repeat rvalue.

Fixes rust-lang/miri#3090

r? @oli-obk

[rustbot]

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

[RalfJung]

This pass is designed to only use global allocations, to avoid any risk of accidental modification of the stored state.

Can you point at where the logic is that realizes this principle?

[bors]

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

[RalfJung]

Suggested change

	/// Intern `ret`, checking it references no other allocation.
	/// Intern `ret`, checking it references no other allocation.
	/// The interned allocation will be marked as immutable.

[RalfJung]

I'd almost prefer if this was removed and we'd get an ICE if the check below ever fails -- the caller should ensure there is no provenance, and if it fails to do so we probably want to know (rather than silently failing to const-prop).

[RalfJung]

This path should be unreachable now? Why is there a fallback path here? I can't see why we'd ever want to copy.

[cjgillot]

op could be an immediate scalar pair. That wouldn't be caught by the scalar path, nor the mplace path.

[RalfJung]

Seems nicer to have a 3-way match than a series of fallbacks, then.

[cjgillot]

For scalars, we may want to encode them in MIR as a ConstValue::Scalar, even if the interpreter backs it by a MPlace.

[RalfJung]

Yeah sure. What I dislike is to have this look like "try A; try B; try C", I'd much rather have a very clear case distinction showing when we use which case. In particular since the code currently completely mixes up checks that are actually needed and errors that can never happen and certainly shouldn't lead to a fallback.

So you can do something like

• first check if let Abi::Scalar(abi::Scalar::Initialized { .. }) = op.layout.abi. In that case always use ConstValue::Scalar, never fall back to anything else, with a comment explaining why we are doing this.
• otherwise match on op.as_mplace_or_imm(), and if you have an mplace use the fast path, otherwise the copy, with a comment explaining this can happen for ScalarPair and ideally we'd have a more efficient representation for them but currently we don't (Cc Further possibilities for mir::Constant cleanup #115877)

[bors]

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

[oli-obk]

can this be violated? Could we just assert that they are equal?

[cjgillot]

This cannot happen from surface rust, as MIR is built such as a switch on the discriminant dominates the downcasts.
Reading reference chapter on enums and RFC 2195, I understand that accessing the wrong variant is not UB if the enum has repr. So it's not impossible for a series of MIR opts to generate a downcast to an inactive variant. IIUC, miri does not check the variant either.
This can definitely happen from custom MIR. I'd rather silently fail to optimize (which is safe) than ICE.

[oli-obk]

This cannot happen from surface rust, as MIR is built such as a switch on the discriminant dominates the downcasts.
Reading reference chapter on enums and RFC 2195, I understand that accessing the wrong variant is not UB if the enum has repr. So it's not impossible for a series of MIR opts to generate a downcast to an inactive variant.

this could be copied verbatim as a comment on the == expression

[cjgillot]

More pragmatically, this pass is not aware of control-flow. We could have:

_1 = None
_2 = discriminant(_1)
SwitchInt(_2) -> [0: bb1, otherwise: bb2]

bb1:
// stuff

bb2:
_3 = (_1 as Some).0
// stuff

We must not ICE for the assignment to _3.

[oli-obk]

r=me after rebase

[cjgillot]

@bors r=oli-obk

[bors]

📌 Commit 50448d1 has been approved by oli-obk

It is now in the queue for this repository.

[matthiaskrgr]

@bors rollup=never for bisectability

[RalfJung]

Seems nicer to let intern_const_alloc_for_constprop do this check, which naturally has access to the allocation anyway.

It's a bug if this ever fails, right? GVN doesn't do stuff on pointers?

[cjgillot]

We definitely need to handle some pointers. For instance when accessing a static to read from it.
What we cannot do is write pointers in the resulting MIR, because codegen does not respect identity of alloc-id in a across crates.

[RalfJung]

But here this hasn't about "handling" pointers, it's about the constants that are actually tracked as "numbered" expressions and written back into the MIR, right? Can those ever contain pointers?

[cjgillot]

Did you change your mind from #116270 (comment)? I can revert ac4572f

[cjgillot]

The practical issue I have is this one:

static A: fn() = || {};

// In another crate.
fn foo() -> fn() {
    A
}

// In another crate.
fn bar() {
  assert_eq!(A, foo());
}

Consider:

• A is at address alloc1;
• *alloc1 is the pointer alloc2;
• if the opt writes alloc2 in the MIR for foo, the assertion in bar may fail because codegen does not guarantee equality of fn pointers, which is a miscompilation.

On the other hand, for

static B: &'static &'static u8 = &&63;

fn blah() -> u8 {
    **A
}

We definitely want to optimize blah to return 63.

[RalfJung]

They can contain pointers when being numbered, but will not be written back if they do.

Okay, that was my wrong assumption then. So for **A to be turned into 63, we need to number A and *A?

However, wouldn't we want to also optimize *A to be a pointer directly into the allocation that contains &63? That should also be sound, right? The issue isn't pointers in general, it's pointers created by certain operations: consts, and fn-item-to-fn-ptr coercions. So the more principled approach seems to be to never number fn-item-to-fn-ptr coercions, and when encountering a const to check whether it contains any provenance before numbering it. Once we do that any provenance we still number is fine?

Though I'm not entirely sure about statics with weak linkage. So maybe the point is you want to make a wide berth around this entire problem area even if that's unnecessarily conservative?

Or is the point you want to number **A even when A is a const rather than a static, and that requires numbering A? So, the principled approach would be to mark some numbered values as "cannot be written back due to ptr non-determinism".

Either way this needs comments in the code that explain the why and what.

[RalfJung]

Fundamentally, the problem is introducing more non-determinism. So when we write back we must not write back something that could have non-deterministic choice where there was no non-deterministic choice before.

What are the actually non-deterministic operations? An unevaluated constant can have non-determinism, so we can never replace x by C. However, we can replace x by the result obtained from evaluating C! An evaluated constant doesn't have extra non-determinism in general. So is this one even a problem for GVN, does GVN track unevaluated constants symbolically? If no, there should probably be a comment somewhere explaining why that would be a problem.

And then there's function pointers. Those are nasty indeed since even the same evaluated result can lead to different values at runtime, I think. The way to protect against that, I think, is to never number a value that contains provenance that points to a function allocation (or a vtable allocation... I'm not sure if we currently mark them as "address insignificant" but it seems like a reasonable thing to do).

If we do that, then we ensure that bad values never enter the GVN system, and then everything that we do number can be put back into the program, right?

[cjgillot]

You are right. I found an example of erroneous merging of constants.

fn opaque<T>(x: T) -> T { x }

#[custom_mir(dialect="analysis")]
fn foo() -> (bool, bool) {
    mir!(
        let au: u128;
        let bu: u128;
        let cu: u128;
        let du: u128;
        let c: &str;
        let d: &str;
        {
            let a = ("a",);
            Call(au = transmute::<_, u128>(a.0), bb1)
        }
        bb1 = {
            Call(c = opaque(a.0), bb2)
        }
        bb2 = {
            Call(cu = transmute::<_, u128>(c), bb3)
        }
        bb3 = {
            let b = "a";
            Call(bu = transmute::<_, u128>(b), bb4)
        }
        bb4 = {
            Call(d = opaque(b), bb5)
        }
        bb5 = {
            Call(du = transmute::<_, u128>(d), bb6)
        }
        bb6 = {
            let direct = au == bu;
            let indirect = cu == du;
            RET = (direct, indirect);
            Return()
        }
    )
}

Normally, both direct and indirect hold the same value. The current implementation merges au and bu, so direct is always true, but fails to merge a.0 and b so cu and du may be different. Miri fails when enabling GVN on this case.

[RalfJung]

So specifically what is happening here is numbering of unevaluated consts, with subsequent duplication of the non-determinism that arises from their evaluation?

[cjgillot]

Yes. We give the same VnIndex to a.0 and b, although miri/codegen is allowed to give them different AllocIds. The latest push adds a disambiguation mechanism for non-deterministic constants.

[RalfJung]

r=oli-obk,me with some last comment fixes and squashing the history a bit.

I'm not at all happy with the amount of hacks here. Our const infrastructure is just not ready for the kind of heavy lifting this pass does. But I guess that shouldn't block experimenting with the GVN pass... it just makes reviewing (and writing) such passes so much more painful. I'd rather not spend many more hours reviewing similar changes, instead we should clean up the const infrastructure such that these reviews become simpler.

[RalfJung]

@bors r=oli-obk,RalfJung

[bors]

📌 Commit 24be433 has been approved by oli-obk,RalfJung

It is now in the queue for this repository.

[bors]

⌛ Testing commit 24be433 with merge 83c9732...

[bors]

☀️ Test successful - checks-actions
Approved by: oli-obk,RalfJung
Pushing 83c9732 to master...

[rust-timer]

Finished benchmarking commit (83c9732): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.5%	[0.7%, 4.3%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 635.492s -> 633.735s (-0.28%)
Artifact size: 304.42 MiB -> 304.43 MiB (0.00%)