[PATCH] D143646: [RISCV] Return false from shouldFormOverflowOp

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:8923
+  ISD::CondCode Cond = cast<CondCodeSDNode>(N->getOperand(2))->get();
+  if (OpVT == MVT::i64 && !Subtarget.is64Bit() && Cond == ISD::SETULT &&
+      N0->getOpcode() == ISD::ADD && !isa<ConstantSDNode>(N1) &&
----------------
liaolucy wrote:
> craig.topper wrote:
> > I don't understand this criteria. Just looking at the operands of the add is arbitrary. In your example, it seems like the select is important, but you don't check for that.
> It should have nothing to do with the select instruction.
> In fact selectDAG is just one more instruction than before:  %10:gpr = SLTU %6:gpr, %0:gpr 
> IR Dump After Machine code sinking (machine-sink) : generating redundant branch instruction + copy SLTU 
> 
> when add 64 is expanded, it is compared with the left side by default.
> https://github.com/llvm/llvm-project/blob/main/llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp#L3017
> ```
> Optimized lowered selection DAG: %bb.0 
> 'uaddo1_math_overflow_used:'
> SelectionDAG has 29 nodes:
>   t0: ch,glue = EntryToken
>     t2: i32,ch = CopyFromReg t0, Register:i32 %0
>     t4: i32,ch = CopyFromReg t0, Register:i32 %1
>   t11: i64 = build_pair t2, t4
>     t6: i32,ch = CopyFromReg t0, Register:i32 %2
>     t8: i32,ch = CopyFromReg t0, Register:i32 %3
>   t12: i64 = build_pair t6, t8
>   t13: i64 = add t12, t11
>     t15: i1 = setcc t13, t11, setult:ch
>   t17: i64 = select t15, t12, Constant:i64<42>
>       t10: i32,ch = CopyFromReg t0, Register:i32 %4
>     t20: ch = store<(store (s64) into %ir.res)> t0, t13, t10, undef:i32
>     t23: i32 = extract_element t17, Constant:i32<0>
>   t25: ch,glue = CopyToReg t20, Register:i32 $x10, t23
>     t22: i32 = extract_element t17, Constant:i32<1>
>   t27: ch,glue = CopyToReg t25, Register:i32 $x11, t22, t25:1
>   t28: ch = RISCVISD::RET_FLAG t27, Register:i32 $x10, Register:i32 $x11, t27:1
> 
> 
> Type-legalized selection DAG: %bb.0 'uaddo1_math_overflow_used:'
> SelectionDAG has 38 nodes:
>   t0: ch,glue = EntryToken
>   t2: i32,ch = CopyFromReg t0, Register:i32 %0
>   t4: i32,ch = CopyFromReg t0, Register:i32 %1
>   t6: i32,ch = CopyFromReg t0, Register:i32 %2
>   t8: i32,ch = CopyFromReg t0, Register:i32 %3
>   t10: i32,ch = CopyFromReg t0, Register:i32 %4
>       t44: ch = store<(store (s32) into %ir.res, align 8)> t0, t30, t10, undef:i32
>         t46: i32 = add nuw t10, Constant:i32<4>
>       t47: ch = store<(store (s32) into %ir.res + 4, basealign 8)> t0, t33, t46, undef:i32
>     t48: ch = TokenFactor t44, t47
>     t35: i32 = select t38, t6, Constant:i32<42>
>   t25: ch,glue = CopyToReg t48, Register:i32 $x10, t35
>     t36: i32 = select t38, t8, Constant:i32<0>
>   t27: ch,glue = CopyToReg t25, Register:i32 $x11, t36, t25:1
>   t30: i32 = add t6, t2
>     t31: i32 = add t8, t4
>     t32: i32 = setcc t30, t6, setult:ch -------- if change to :  t32: i32 = setcc t30, t2, setult:ch
>   t33: i32 = add t31, t32
>       t42: i32 = setcc t33, t4, seteq:ch
>       t39: i32 = setcc t30, t2, setult:ch. -------  t32 and t42 are the same， t32/t42 can be delete.
>       t40: i32 = setcc t33, t4, setult:ch
>     t43: i32 = select t42, t39, t40
>   t38: i32 = and t43, Constant:i32<1>
>   t28: ch = RISCVISD::RET_FLAG t27, Register:i32 $x10, Register:i32 $x11, t27:1
> ```
Ok I understand now. I'm concerned there aren't enough checks here to prevent infinite loops. If there are two setccs that both use the add but compare to a different operand, we'll infinite loop commuting the add.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143646