[PATCH] D62266: [DAGCombine][X86][AArch64][ARM] (C - x) + y -> (y - x) + C fold

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri marked an inline comment as done.
lebedev.ri added inline comments.


================
Comment at: test/CodeGen/ARM/addsubcarry-promotion.ll:35
+; THUMBV6M-NEXT:    ldr r1, .LCPI0_0
+; THUMBV6M-NEXT:    cmp r0, r1
+; THUMBV6M-NEXT:    beq .LBB0_2
----------------
lebedev.ri wrote:
> lebedev.ri wrote:
> > lebedev.ri wrote:
> > > lebedev.ri wrote:
> > > > efriedma wrote:
> > > > > The big difference that's making the new code worse, now, is that somehow the compare `x==0` is getting transformed to something more like `x==-1`... and I guess something isn't handling that well. That's probably something the ARM backend should be handling in target-specific code, though; feel free to just file a bug for the missed optimization on `void a(int s, void f()) { if ((short)s==-1)f(); }`
> > > > I understand how we get here. In IR terms, this is just two folds:
> > > > https://rise4fun.com/Alive/fTpN
> > > > https://godbolt.org/z/6RLVTQ
> > > > 
> > > > And thus we end with:
> > > > ```
> > > > Optimized type-legalized selection DAG: %bb.0 'fn1:entry'
> > > > SelectionDAG has 20 nodes:
> > > >   t0: ch = EntryToken
> > > >               t6: i32,ch = CopyFromReg t0, Register:i32 %2
> > > >             t37: i32 = sub Constant:i32<0>, t6
> > > >           t39: i32 = sign_extend_inreg t37, ValueType:ch:i16
> > > >             t4: i32,ch = CopyFromReg t0, Register:i32 %1
> > > >             t2: i32,ch = CopyFromReg t0, Register:i32 %0
> > > >           t36: i32,i32 = uaddo t4, t2
> > > >         t47: i32,i32 = addcarry t39, Constant:i32<0>, t36:1
> > > >       t46: i32 = and t47, Constant:i32<65535>
> > > >     t50: ch = br_cc t0, seteq:ch, t46, Constant:i32<65535>, BasicBlock:ch<if.end 0x5618ef707f18>
> > > >   t19: ch = br t50, BasicBlock:ch<for.cond.preheader 0x5618ef707d88>
> > > > ```
> > > > 
> > > > I wonder, maybe this can still be undone in DAGCombine.
> > > > The important bits are:
> > > > ```
> > > >         t47: i32,i32 = addcarry t39, Constant:i32<0>, t36:1
> > > >       t46: i32 = and t47, Constant:i32<65535>
> > > >     t50: ch = br_cc t0, seteq:ch, t46, Constant:i32<65535>, BasicBlock:ch<if.end 0x5618ef707f18>
> > > > ```
> > > > I wonder if the problem can be fixed by transforming this back into:
> > > > ```
> > > >       t47: i32,i32 = addcarry t39, Constant:i32<65535>, t36:1
> > > >     t50: ch = br_cc t0, seteq:ch, t47, Constant:i32<0>, BasicBlock:ch<if.end 0x5618ef707f18>
> > > > ```
> > > > 
> > > Or more specifically, https://rise4fun.com/Alive/slGX
> > > This would be an ok transform here because the second argument of `addcarry` is 0 here anyway.
> > Or without hardcoded constants: https://rise4fun.com/Alive/B2k
> > Not sure if it's worthwhile doing this if it requires creating whole new 'add',
> > but here we should be able to do it.
> Done, D62450.
> I think that fixes (and improves upon?) the last regression, feel free to review these three patches :)
ping @efriedma :)


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62266