Match on two-variant enum optimizes poorly

[clubby789]

https://godbolt.org/z/f3TMe3rcW

good gets optimal codegen since the LLVM 18 bump, but bad has had poor codegen for some time. Codegen also becomes optimal if a and b are references rather than passed by value.

[clubby789]

Not sure if this is necessarily A-LLVM actually; it looks like we don't give enough information to codegen to improve this

We actually do:

%0 = alloca ...
%1 = load i8, ptr %0, align 1, !range <range info>

Somewhere in SROA that range information seems to get lost

[erikdesjardins]

Codegen also becomes optimal if a and b are references

Looks like this is because of the range metadata on the load: https://godbolt.org/z/bPfezGhW6.

So this may be fixed after LLVM 19, since we'll be able to put range metadata on the by-value arguments.

(range on arguments has been merged upstream, but optimization support is being implemented right now, so the following isn't optimized yet, but it may be soon: https://godbolt.org/z/17s6jTfMv)

[dianqk]

#122726 should be a similar issue.

[dianqk]

So this may be fixed after LLVM 19, since we'll be able to put range metadata on the by-value arguments.

(range on arguments has been merged upstream, but optimization support is being implemented right now, so the following isn't optimized yet, but it may be soon: https://godbolt.org/z/17s6jTfMv)

@erikdesjardins Are you adding relevant support? Or someone has started.

I'm curious why we don't put the range metadata on the non-load instruction.

[dianqk]

Reference: llvm/llvm-project#83171.

[erikdesjardins]

Yeah, that's the original PR that added support, and there have been a few follow ups so far to have different passes check for range attributes. Since there's still quite a while until LLVM 19, I imagine that support will be added to every pass or analysis that currently uses range metadata by that time.

I'm curious why we don't put the range metadata on the non-load instruction.

LLVM generally only checks for such metadata on loads and calls (e.g.), so I think adding it to other instructions won't have much of an effect.

^{The only usage that allows any kind of instruction appears to be here in InstSimplify. It's used only to optimize comparisons that are always true or always false.}

[dianqk]

I'm curious why we don't put the range metadata on the non-load instruction.

LLVM generally only checks for such metadata on loads and calls (e.g.), so I think adding it to other instructions won't have much of an effect.

IIUC, we can add range metadata to other instructions. We just don't do that now. We can quickly get the range of arbitrary values when the source value provides range metadata. Adding range metadata for other instructions doesn't seem necessary if that's correct.

[scottmcm]

Somewhere in SROA that range information seems to get lost

I'm pretty sure that's because we store to the alloca then load from it again. So a very early pass replaces the load-store with just using the argument, but in doing so doesn't preserve the !range metadata anywhere.

It'd need to insert an assume of some sort to keep it, and that has enough other issues that LLVM usually doesn't do that, AFAIK.

[dianqk]

Somewhere in SROA that range information seems to get lost

I'm pretty sure that's because we store to the alloca then load from it again. So a very early pass replaces the load-store with just using the argument, but in doing so doesn't preserve the !range metadata anywhere.

It'd need to insert an assume of some sort to keep it, and that has enough other issues that LLVM usually doesn't do that, AFAIK.

I've tried it on #122728. I got a huge change in the perf result. 🫠

[scottmcm]

I got a huge change in the perf result. 🫠

I'd been thinking about the possibility of LLVM doing the !range → assume translation as part of removing the load. But yeah, that may well have the same perf result problem.

[veera-sivarajan]

@rustbot claim

[veera-sivarajan]

@rustbot label llvm-fixed-upstream

[scottmcm]

bad is still poor as of rustc 1.87.0-nightly (a2e63569f 2025-03-26), even though rustc itself adds nuws now.

But with llvm/llvm-project#133344 fixed upstream, after the LLVM 21 version upgrade we should revisit this and see whether we can enable this test:

rust/tests/codegen/option-niche-eq.rs

Lines 65 to 73 in 2a06022

	// FIXME: This should work too
	// // FIXME-CHECK-LABEL: @bool_eq
	// #[no_mangle]
	// pub fn bool_eq(l: Option<bool>, r: Option<bool>) -> bool {
	// // FIXME-CHECK: start:
	// // FIXME-CHECK-NEXT: icmp eq i8
	// // FIXME-CHECK-NEXT: ret i1
	// l == r
	// }

[scottmcm]

Looks like it'll be enough! With trunk LLVM and the current no-prepopulate-passes IR from rustc, it all collapses: https://llvm.godbolt.org/z/h7TW1qnzT

Still can't go back to the derive, though, since with the derived eq (rather than the custom one on Option), we still get https://llvm.godbolt.org/z/x151E9j3T which is improved from current nightly, but not yet fully optimized.

[scottmcm]

I looked into this some more, and found another LLVM bug: llvm/llvm-project#134024

Looks like it needs at least three variants before it impacts us, so it's not the solution to fixing the derive either :(

---

EDIT: ...and another one for the >2 variants case, llvm/llvm-project#134028