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

[Liao Chunyu via Phabricator via llvm-commits]

liaolucy 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) &&
----------------
craig.topper wrote:
> 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.
Thanks Craig.
I didn't think of a good way to solve this problem and I will remove this code. Probably need to test benchmark to prove if this patch is really needed.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143646