[PATCH] D157772: [RISCV][GISel] Lower G_UADDE, G_UADDO, G_USUBE, and G_USUBO

Mon Aug 14 18:30:51 PDT 2023

craig.topper marked an inline comment as not done.
craig.topper added inline comments.

================
Comment at: llvm/test/CodeGen/RISCV/GlobalISel/legalizer/rv64/legalize-add.mir:110
     ; CHECK-NEXT: %y01:_(s64) = COPY $x13
-    ; CHECK-NEXT: [[UADDO:%[0-9]+]]:_(s64), [[UADDO1:%[0-9]+]]:_(s64) = G_UADDO %x00, %y00
+    ; CHECK-NEXT: [[ADD:%[0-9]+]]:_(s64) = G_ADD %x00, %y00
+    ; CHECK-NEXT: [[ICMP:%[0-9]+]]:_(s64) = G_ICMP intpred(ult), [[ADD]](s64), %y00
----------------
craig.topper wrote:
> @arsenm Can you check this expansion? It doesn't look correct to me.
> 
> Here's the code from LegalizerHelper.
> 
> ```
>     auto TmpRes = MIRBuilder.buildAdd(Ty, LHS, RHS);                             
>     auto ZExtCarryIn = MIRBuilder.buildZExt(Ty, CarryIn);                        
>     MIRBuilder.buildAdd(Res, TmpRes, ZExtCarryIn);                               
>     MIRBuilder.buildICmp(CmpInst::ICMP_ULT, CarryOut, Res, LHS); 
> ```
> 
> If LHS is 0 and RHS is all ones, and CarryIn is 1. TmpRes will be all ones and Res will be 0. Res will be 0. The ICMP_ULT will be false, but we there was an overflow.
Patch to fix G_UADDE legalization https://reviews.llvm.org/D157943

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157772