[SimplifyCFG] handle monotonic wrapped case for D150943

[khei4]

This is a follow-up on the miscompile for wrapping the flag for the switch to the lookup table on SimplifyCFG. Checking the switch results' monotonicity isn't sufficient to say linear map calculation has no wrapping. Monotonicity is only necessary, and further checking whether wrapping happens on the maximum index is required to be sufficient.
For example( @mikaelholmen gave ), https://alive2.llvm.org/ce/z/So_fzw
Total linear map (-2, -1, 1, 2) |→ (-4, -2, 0, 2) is monotonic, but actual calculation contains wrapping (-2, -1, 1, 2) |→ (0, 1, 2, 3) (*2) |→ (0, 2, 4(wrapped), 6(wrapped)) (-4)|→ (-4, -2, 0, 2).

Also on the Linear map calculation, checking the multiplication wrapping with monotonicity is enough.(If the addition is only wrapped not with multiplication, contradicts with monotonicity. )

[mikaelholmen]

Thanks @khei4 !
This looks good to me and I've verified that it solves the original miscompile problem I saw but I'd prefer if @nikic has a look too.

[djkoloski]

We ran into this on Fuchsia, which builds using tip-of-tree rustc. Once this is merged, can we get it added to the llvmorg-17.0.0-rc4 tag, or a new release candidate tag that I can pull the fix back up into rust?

[dianqk]

We ran into this on Fuchsia, which builds using tip-of-tree rustc. Once this is merged, can we get it added to the llvmorg-17.0.0-rc4 tag, or a new release candidate tag that I can pull the fix back up into rust?

Bug fixes are always welcome. If you want to understand the process, refer to https://m.youtube.com/watch?v=onaNb2U1Od8.

[nikic]

Can you please update the PR description with more details about what the problematic case is an how this fixes it?

My understanding is that in this case we are mapping (after index adjustment) the indices (0, 1, 2, 3) to (-4, -2, 0, 2) so we have a monotonic sequence, but this is done by doing a multiply first, so we go through (0, 2, -4, -2) first, which wraps.

[khei4]

Thank you for the review!

Can you please update the PR description with more details about what the problematic case is an how this fixes it?

Sure!

My understanding is that in this case we are mapping (after index adjustment) the indices (0, 1, 2, 3) to (-4, -2, 0, 2) so we have a monotonic sequence, but this is done by doing a multiply first, so we go through (0, 2, -4, -2) first, which wraps.

I think you are right.

[nikic]

LGTM

[djkoloski]

I'm having trouble backporting this to 17.x. I opened #66373 to backport it but it got closed automatically because the 17.x branch does not have the latest changes which remove the repo lockdown. So I opened #66375 to disable the repo lockdown on the 17.x branch but that was also closed automatically (I didn't know that GHA would run using the in-tree actions instead of the ones in the PR). Any suggestions? Should I go through Phabricator for this?

Sorry for posting here, I can't comment on any of the PRs I opened because they're locked.

[nikic]

@djkoloski Backports still follow the process at https://llvm.org/docs/GitHub.html#backporting-fixes-to-the-release-branches.

[dianqk]

@djkoloski You need to create an issue first. The presentation above has more details.
I can do this on the weekends if needed.

[djkoloski]

Thanks for the help everyone, sorry for the confusion. I've created issue #66396 so a release manager can decide whether this change should be added to the LLVM 17.0.X milestone.