[Transforms] LowerSwitch tries to get sext on `i128`, causing an assertion error.

[DataCorrupted]

In LowerSwitch, we are assuming treating switch cases is smaller than int64 by directly asking for a sext value.
However, this can go wrong if the case value is higher typed huge number.
For example, https://godbolt.org/z/f8cMnKnGE shows a case where we use i128 and crashes the compiler.

llvm-project/llvm/lib/Transforms/Utils/LowerSwitch.cpp

Line 331 in 7cf5581

int64_t nextValue = J->Low->getSExtValue();

Here is a stack trace:

llc: /home/peter/aflplusplus-isel/llvm-fix/llvm/include/llvm/ADT/APInt.h:1501: int64_t llvm::APInt::getSExtValue() const: Assertion `getSignificantBits() <= 64 && "Too many bits for int64_t"' failed.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
Stack dump:
0.      Program arguments: ./bin/llc -mtriple=amdgcn crash/amdgcn/0/poc.ll
1.      Running pass 'CallGraph Pass Manager' on module 'crash/amdgcn/0/poc.ll'.
2.      Running pass 'Lower SwitchInst's to branches' on function '@f'
 #0 0x00007f52dcdce4ba llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) /home/peter/aflplusplus-isel/llvm-fix/llvm/lib/Support/Unix/Signals.inc:567:11
 #1 0x00007f52dcdce66b PrintStackTraceSignalHandler(void*) /home/peter/aflplusplus-isel/llvm-fix/llvm/lib/Support/Unix/Signals.inc:641:1
 #2 0x00007f52dcdcccc6 llvm::sys::RunSignalHandlers() /home/peter/aflplusplus-isel/llvm-fix/llvm/lib/Support/Signals.cpp:104:5
 #3 0x00007f52dcdced95 SignalHandler(int) /home/peter/aflplusplus-isel/llvm-fix/llvm/lib/Support/Unix/Signals.inc:412:1
 #4 0x00007f52db58c980 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x12980)
 #5 0x00007f52da888e87 raise /build/glibc-CVJwZb/glibc-2.27/signal/../sysdeps/unix/sysv/linux/raise.c:51:0
 #6 0x00007f52da88a7f1 abort /build/glibc-CVJwZb/glibc-2.27/stdlib/abort.c:81:0
 #7 0x00007f52da87a3fa __assert_fail_base /build/glibc-CVJwZb/glibc-2.27/assert/assert.c:89:0
 #8 0x00007f52da87a472 (/lib/x86_64-linux-gnu/libc.so.6+0x30472)
 #9 0x00007f52dc04bfc5 llvm::APInt::getSExtValue() const /home/peter/aflplusplus-isel/llvm-fix/llvm/include/llvm/ADT/APInt.h:0:5
#10 0x00007f52dc1d2a89 llvm::ConstantInt::getSExtValue() const /home/peter/aflplusplus-isel/llvm-fix/llvm/include/llvm/IR/Constants.h:148:41
#11 0x00007f52dc258a65 (anonymous namespace)::Clusterify(std::vector<(anonymous namespace)::CaseRange, std::allocator<(anonymous namespace)::CaseRange>>&, llvm::SwitchInst*) /home/peter/aflplusplus-isel/llvm-fix/llvm/lib/Transforms/Utils/LowerSwitch.cpp:331:15
#12 0x00007f52dc257dc6 (anonymous namespace)::ProcessSwitchInst(llvm::SwitchInst*, llvm::SmallPtrSetImpl<llvm::BasicBlock*>&, llvm::AssumptionCache*, llvm::LazyValueInfo*) /home/peter/aflplusplus-isel/llvm-fix/llvm/lib/Transforms/Utils/LowerSwitch.cpp:372:18
#13 0x00007f52dc257af3 (anonymous namespace)::LowerSwitch(llvm::Function&, llvm::LazyValueInfo*, llvm::AssumptionCache*) /home/peter/aflplusplus-isel/llvm-fix/llvm/lib/Transforms/Utils/LowerSwitch.cpp:535:3
#14 0x00007f52dc257ceb (anonymous namespace)::LowerSwitchLegacyPass::runOnFunction(llvm::Function&) /home/peter/aflplusplus-isel/llvm-fix/llvm/lib/Transforms/Utils/LowerSwitch.cpp:595:3
#15 0x00007f52df7149e6 llvm::FPPassManager::runOnFunction(llvm::Function&) /home/peter/aflplusplus-isel/llvm-fix/llvm/lib/IR/LegacyPassManager.cpp:1430:23
#16 0x00007f52e1fa9eed (anonymous namespace)::CGPassManager::RunPassOnSCC(llvm::Pass*, llvm::CallGraphSCC&, llvm::CallGraph&, bool&, bool&) /home/peter/aflplusplus-isel/llvm-fix/llvm/lib/Analysis/CallGraphSCCPass.cpp:179:20
#17 0x00007f52e1fa987e (anonymous namespace)::CGPassManager::RunAllPassesOnSCC(llvm::CallGraphSCC&, llvm::CallGraph&, bool&) /home/peter/aflplusplus-isel/llvm-fix/llvm/lib/Analysis/CallGraphSCCPass.cpp:476:10
#18 0x00007f52e1fa91ff (anonymous namespace)::CGPassManager::runOnModule(llvm::Module&) /home/peter/aflplusplus-isel/llvm-fix/llvm/lib/Analysis/CallGraphSCCPass.cpp:542:18
#19 0x00007f52df7152b9 (anonymous namespace)::MPPassManager::runOnModule(llvm::Module&) /home/peter/aflplusplus-isel/llvm-fix/llvm/lib/IR/LegacyPassManager.cpp:1545:23
#20 0x00007f52df714e2d llvm::legacy::PassManagerImpl::run(llvm::Module&) /home/peter/aflplusplus-isel/llvm-fix/llvm/lib/IR/LegacyPassManager.cpp:535:16
#21 0x00007f52df719af1 llvm::legacy::PassManager::run(llvm::Module&) /home/peter/aflplusplus-isel/llvm-fix/llvm/lib/IR/LegacyPassManager.cpp:1672:3
#22 0x00000000004199ac compileModule(char**, llvm::LLVMContext&) /home/peter/aflplusplus-isel/llvm-fix/llvm/tools/llc/llc.cpp:736:41
#23 0x0000000000417d52 main /home/peter/aflplusplus-isel/llvm-fix/llvm/tools/llc/llc.cpp:417:13
#24 0x00007f52da86bc87 __libc_start_main /build/glibc-CVJwZb/glibc-2.27/csu/../csu/libc-start.c:344:0
#25 0x000000000041755a _start (./bin/llc+0x41755a)
Aborted

I see two ways to fix it:

1. Change the documentation and clarify that switch case can be at most int64_t and will be treated as signed integer.
2. Change the whole file with APInt API.

[llvmbot]

@llvm/issue-subscribers-bug

[rotateright]

I think option 2 is the only viable solution. LowerSwitch is not part of the default set of codegen passes; only enabled for amdgcn? Otherwise, this bug would have been noticed sooner.

[rotateright]

Another potential option (but I have no idea if it's viable): just bail out on switches with oversized type. Presumably, the default switch lowering can handle those minimally (otherwise every target would be crashing). Since oversized switches were never expected to exist for targets that use this pass, it shouldn't be a real-world concern as long as we don't crash?

[DataCorrupted]

I am not really comfortable with bail out solution. That doesn't fit into the expectation of this pass (unless we document it explictly), other people may use it in the future and expecting it to work on i128 when in fact we bail, that could be a bigger problem.

I am happy to apply option 2 when I have time.

[DataCorrupted]

I think there is another discussion here I would like to talk about. @RKSimon

It seems to me APInt exists to represent integers with (probably) higher precisions than 64.
API like getSExtValue or getZExtValue that assumes the integer is less than 64bit or crash, this doesn't really make sense.
I am thinking if we should make some changes to that.

Here's one idea: getSExtValue returns std::optional<int64_t>, so a) this api doesn't crash on arbitrary integer, and b) we force the user to think about the bitwidth before they call it.
For now it seems people are just calling it without thinking twice (like this file).

[RKSimon]

There might be uses for an alternative helper (getLegalSExtValue?) but the existing getSExtValue/getZExtValue methods are very widespread and make use of being implicitly inrange.

[RKSimon]

Something else that has encouraged people to use getSExtValue/getZExtValue is because APInt values don't work well with other APInt values if their bitwidths aren't the same - IMO at least the non-equality comparisons shouldn't have that limit.

[DataCorrupted]

There might be uses for an alternative helper (getLegalSExtValue?) but the existing getSExtValue/getZExtValue methods are very widespread and make use of being implicitly inrange.

I wonder what would 'getLegalSExtValue' return, an option like I mentioned?

I think another idea is to roll out new, safe APIs and try to slowly deprecate the old ones, if this API is widely used.