Added codegen tests for different forms of Option::or

[rwardd]

Adds tests to check the output of the different ways of writing Option::or

Fixes #124533

[rustbot]

Mark-Simulacrum is not on the review rotation at the moment.
They may take a while to respond.

[rustbot]

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
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

[scottmcm]

r? scottmcm

[scottmcm]

Thanks for picking up this work!

Looking mostly good; I left some comments. The DAG one I do feel reasonably strongly about. The others think about and feel free to push back if you think otherwise.

View changes since this review

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[rwardd]

The DAG one I do feel reasonably strongly about. The o

Thank you very much for the valuable feedback - I agree with all the comments, and have attempted to address and implement all of it. I have two further questions:

1. If/when this PR gets approved, are you happy if the same style is followed to build out the Option::and and Result::{and, or} tests? E.g., have multiple functions for the different forms of the operation, and test them against the three types (u8, [u8; 1], NonZero<u8>) as was done in this PR?

2. Is there value in writing tests for larger types (e.g. u64, [u64; 1] ...)? I am cognisant of keeping tests minimal as stated in the Best Practices section, and don't want to inflate the tests if they are superfluous.

Thanks again!

[rwardd]

@rustbot ready

[scottmcm]

1. Yes, this same style makes sense to me.
2. Unless there's a particular reason to think they'll do something different, I'd prefer fewer tests overall. LLVM support arbitrary-sized integer types (even i40 and such), which means it's usually pretty reliable at not doing something different just because it's a wider integer type. If you play around and notice a difference that's interesting, then sure, but hopefully the "what's the BackendRepr variant of the option" coverage is getting the interesting differences and thus different widths (or floats, or ...) aren't interesting enough to bother testing separately. (Unless, of course, someone finds an issue about them.)

[scottmcm]

Thanks for adding these! Nice to see the argument names and such too, since it's short so that's not a big deal.

But that ret on its own isn't actually checking anything. How about, instead,

Suggested change

	pub fn or_match_nz_u8(opta: Option<NonZero<u8>>, optb: Option<NonZero<u8>>) -> Option<NonZero<u8>> {
	// CHECK: start:
	// CHECK-NEXT: [[NOT_A:%.*]] = icmp eq i8 %0, 0
	// CHECK-NEXT: select i1 [[NOT_A]], i8 %optb, i8 %0
	// ret i8
	pub fn or_match_nz_u8(opta: Option<NonZero<u8>>, optb: Option<NonZero<u8>>) -> Option<NonZero<u8>> {
	// CHECK: start:
	// CHECK: [[NOT_A:%.+]] = icmp eq i8 %0, 0
	// CHECK: [[R:%.+]] = select i1 [[NOT_A]], i8 %optb, i8 %0
	// CHECK: ret i8 [[R]]

Since naming the values means that we're not concerned about other things showing up in the middle, and thus don't need to use the -NEXT that the ones looking at just the instructions do.

[scottmcm]

(And of course ditto the others below)

[scottmcm]

Another round of suggestions 🙃

The most important one is about the un-CHECKed ret comments, but ponder the others as well.

Thanks for making the previous updates! Hopefully these are the last things I'll think of.

View changes since this review

[rwardd]

@rustbot ready

Thank you very much again for feedback (and patience!). I've implemented all the suggestions (specifically ensuring all ret comments are actually CHECKed), and gone ahead and extended some of the existing CHECKs to be more explicit with argument naming and return values.