added some new test to check for result and options opt

[heiseish]

Apologies for the delay. Finally have some time to get back into contributing.

Context

• Added some tests to show optimization on result and options for 64 and 128 bits
• Relevant issue match-then-remake Result doesn't optimize away for some payload widths #101210

Some newb questions from me

• My local llvm IR has nuw in result_nop_match_128 etc whereas godbolt version doesn't have. So I put optional there, but not sure if it's desirable (maybe I'm not using the compiled llvm in the repo). I ran the test with

./x test tests/codegen/try_question_mark_nop.rs

• Unless I'm reading it wrongly, but option_nop_match_128 and option_nop_traits_128 look to be not optimized away?

Update:
Here's the test for future reference

// CHECK-LABEL: @option_nop_match_128
#[no_mangle]
pub fn option_nop_match_128(x: Option<i128>) -> Option<i128> {
    // CHECK: start:
    // CHECK-NEXT: %trunc = trunc nuw i128 %0 to i1
    // CHECK-NEXT: br i1 %trunc, label %bb3, label %bb4
    // CHECK: bb3:
    // CHECK-NEXT: %2 = getelementptr inbounds {{(nuw )?}}i8, ptr %_0, i64 16
    // CHECK-NEXT: store i128 %1, ptr %2, align 16
    // CHECK: bb4:
    // CHECK-NEXT: %storemerge = phi i128 [ 1, %bb3 ], [ 0, %start ]
    // CHECK-NEXT: store i128 %storemerge, ptr %_0, align 16
    // CHECK-NEXT: ret void
    match x {
        Some(x) => Some(x),
        None => None,
    }
}

r? @scottmcm

[scottmcm]

Sorry for being slow to get to this!

Yes, GEP nuw is a pretty new thing, and differs between LLVM versions. Since we're not caring about the details of the GEPs in this test, making them optional is the right thing.

option_nop_match_128

Hmm, yeah, this is interesting. I think it's a combination of two things:

• Because it's an option, %1 isn't noundef the way the it is in the result/controlflow cases
• Because it's so big, it's no longer returned in registers but in an sret, compared to things like option_nop_traits_64

I guess between those it doesn't want to write possibly-undef to the sret unless it knows it should.

Maybe just leave those out for now? I opened #138332 that maybe we should be using a different layout for them anyway.

[scottmcm]

Thanks for picking this up! It's great to see that LLVM's gotten smarter.

[scottmcm]

Please avoid hard-coding value names in here, because (unless they're parameters or things we directly set in rustc) it makes the tests fragile as the name changes. This doesn't care about it being named specifically %.2 for example, so it'd be good to use a variable for that instead.

Similarly for %trunc, so please update to something along the lines of

Suggested change

	// TWENTY-NEXT: %trunc = trunc nuw i64 %0 to i1
	// TWENTY-NEXT: %.2 = select i1 %trunc, i64 %1, i64 undef
	// TWENTY-NEXT: %[[IS_SOME:.+]] = trunc nuw i64 %0 to i1
	// TWENTY-NEXT: %[[PAYLOAD:.+]] = select i1 %[[IS_SOME]], i64 %1, i64 undef

and similarly in the other tests.

(The %0 and %1 parameters are fine without needing variables like this, though.)

[heiseish]

Thanks Scott! I have updated the tests. Please do take a look again

[heiseish]

@rustbot ready

☃

[scottmcm]

Looks good, thanks!

@bors r+ rollup=iffy (would be nice if it rolls up, but it's risky because new codegen test changes, so up to your judgment)

[bors]

📌 Commit bcbb199 has been approved by scottmcm

It is now in the queue for this repository.

[bors]

⌛ Testing commit bcbb199 with merge 668bf13...

[bors]

💔 Test failed - checks-actions

[scottmcm]

From a quick scan it looks like that failure might be legit?
@bors r-

[heiseish]

Looks like it. Will take a look

[heiseish]

Silly mistake on my part. Will fix and run tests for both llvm versions when I got home later. Thanks!

[heiseish]

@rustbot ready

[scottmcm]

Thanks for the updates! Let's try it again...
@bors r+

[bors]

📌 Commit 0fba203 has been approved by scottmcm

It is now in the queue for this repository.

[bors]

⌛ Testing commit 0fba203 with merge 5f3b84a...

[bors]

☀️ Test successful - checks-actions
Approved by: scottmcm
Pushing 5f3b84a to master...

[github-actions]

This is an experimental post-merge analysis report. You can ignore it.

Post-merge report

Comparing 66678e6 (base) -> 5f3b84a (this PR)

Test differences

No test diffs found

[rust-timer]

Finished benchmarking commit (5f3b84a): 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 (primary -0.0%)

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)	2.1%	[2.1%, 2.1%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.2%	[-2.2%, -2.2%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.0%	[-2.2%, 2.1%]	2

Cycles

Results (secondary -6.3%)

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)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-6.3%	[-6.5%, -6.1%]	2
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 775.015s -> 775.444s (0.06%)
Artifact size: 365.11 MiB -> 365.12 MiB (0.00%)