[PATCH] D139238: [ARM] IselLowering unsigned overflow to crash using APInt in PerformSHLSimplify

[Peter Rong via Phabricator via llvm-commits]

Peter added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:13777
+  uint64_t C2Value = C2Int.getZExtValue();
+  if (C2Width <= C2Value) 
+    return SDValue();
----------------
RKSimon wrote:
> I'm not sure if you can encounter types > 64 bits here - but you might be better doing the test as 
> ```
> if (C2Int.uge(C2Width))
>   return SDValue();
> uint64_t C2Value = C2Int.getZExtValue();
> ```
I 100% agree with you. I meant to change that in another patch as it could be some other issue, but I'll just do it here.
However, I couldn't find a file that can crash it, probably legalizer has already split i128 into smaller types prior to this, so I'll just update the code without new tests.

`getZExtValue` and `getSExtValue` in APInt can be particularly problematic, I see people use it without checking if it is allowed and crash the compiler.
I have found another bug with it that actually fits your concern. (https://github.com/llvm/llvm-project/issues/59316)
Probably we should move this discussion to there.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139238/new/

https://reviews.llvm.org/D139238