Could you comment what this function does?

Done.

Calling as_mut() clears the CFG caches unconditionally. Is there a way to delay this invalidation?

I submitted a separate commit. I invalidate the cache only after modifying the CFG. If the cache is used later in a loop, we need to invalidate it after each modification.

With MirPatch we don't need to worry about this.

IIUC, this transform is a strict superset of SimplifyToIf, isn't it? Can we only keep this one?

Most of their code can't be shared, and if they were put together, I'd be worried that it would be difficult to maintain later. A major difference is that the otherwise target of SimplifyToIf is reachable, while SimplifyToExp is unreachable.

What is the point of doing the comparison in such a complicated way? Why turn r into a ScalarInt and then back into a i128?

Because SwitchTargets only saves the value of the corresponding bit value by u128, it lacks information on bit-width and sign.

It doesn't need the sign, as I said: == is sign-independent, so we can figure out the switch target without knowing the sign. And it has the width, it is determined by the type of the match operand.

Ah, you're right. This code is used to compare signed integers of different widths, such as i8 and i16. We must add additional conversions for this scenario.
Hmm, it's been just a month, and I have to carefully review the code to respond correctly. It seems I indeed wrote some hard-to-understand code. :>
I'll add some comments. I should also move the unsigned comparison to the front, as I expect this might make the code a bit faster.

This is very strange. == is sign-independent, so I don't understand why both cases need to be considered here. Furthermore, if the sign mattered, then surely it makes no sense to check they_are_equal_signed || they_are_equal_unsigned; instead you have to check if they_are_signed { they_are_equal_signed } else { they_are_equal_unsigned }. Finally, f_c.const_.ty().is_signed() || discr_ty.is_signed() sounds like you are mixing signed and unsigned values (as in, LHS and RHS can have different sign), which should never happen.

What is going on here?

This is used to handle conversions such as from enum(u32) to i32 or from enum(i32) to u32:

Aha... I have no idea what this means. ;)

But it makes no sense to compare things twice here. == is entirely based on being equal bitwise, so the sign doesn't matter.

Same as above (different width), corresponding test case:

But you are comparing both signed and unsigned representation completely disregarding whether the value is actually signed or unsigned. It sounds like you want two cases

• value is signed; then convert everything to i128 (with sign extension, e.g. via try_to_int) and compare there
• value is unsigned; the convert everything to u128 and compare there

But currently you're interpreting the number both ways and then checking if either comparison succeeds. It seems to me that can sometimes lead to blatantly wrong results, e.g. when two numbers are equal unsigned but different after sign extension, and they are actually signed -- you code will treat them as equal, I think?

Ah, I'm fixing it right now.

Fixed in #124122, thanks for pointing it out.