[PATCH] D124743: [DAGCombine] Add node in the worklist in topological order in CombineTo

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/test/CodeGen/AArch64/arm64-abi-varargs.ll:23
+; CHECK-NEXT:    str w9, [sp, #16]
+; CHECK-NEXT:    ldr w9, [x8], #8
 ; CHECK-NEXT:    str x8, [sp, #24]
----------------
deadalnix wrote:
> Pre DAG:
> ```
> SelectionDAG has 55 nodes:
>   t0: ch = EntryToken
>   t19: i32,ch = load<(load (s32) from %fixed-stack.1, align 16)> t0, FrameIndex:i64<-1>, undef:i64
>     t2: i64,ch = CopyFromReg t0, Register:i64 %0
>   t51: ch = store<(store (s32) into %ir.a1)> t19:1, t19, t2, undef:i64
>   t98: i32,ch = load<(load (s32) from %fixed-stack.0, align 8)> t51, FrameIndex:i64<-2>, undef:i64
>     t84: i64 = add FrameIndex:i64<-2>, Constant:i64<8>
>   t109: i32,ch = load<(load (s32) from %fixed-stack.0 + 8, align 8)> t51, t84, undef:i64
>     t108: i64 = add FrameIndex:i64<-2>, Constant:i64<16>
>   t121: i32,ch = load<(load (s32) from %fixed-stack.0 + 16, align 8)> t51, t108, undef:i64
>       t128: ch = store<(store (s32) into %ir.a12)> t0, t121, FrameIndex:i64<12>, undef:i64
>         t120: i64 = add FrameIndex:i64<-2>, Constant:i64<24>
>       t116: ch = store<(store (s64) into %ir.args)> t0, t120, FrameIndex:i64<9>, undef:i64
>       t124: ch = store<(store (s32) into %ir.a11)> t0, t109, FrameIndex:i64<11>, undef:i64
>       t131: ch = store<(store (s32) into %ir.a10)> t0, t98, FrameIndex:i64<10>, undef:i64
>         t16: i32,ch = CopyFromReg t0, Register:i32 %7
>       t53: ch = store<(store (s32) into %ir.8)> t0, t16, FrameIndex:i64<7>, undef:i64
>         t14: i32,ch = CopyFromReg t0, Register:i32 %6
>       t56: ch = store<(store (s32) into %ir.7)> t0, t14, FrameIndex:i64<6>, undef:i64
>         t12: i32,ch = CopyFromReg t0, Register:i32 %5
>       t59: ch = store<(store (s32) into %ir.6)> t0, t12, FrameIndex:i64<5>, undef:i64
>         t10: i32,ch = CopyFromReg t0, Register:i32 %4
>       t62: ch = store<(store (s32) into %ir.5)> t0, t10, FrameIndex:i64<4>, undef:i64
>         t8: i32,ch = CopyFromReg t0, Register:i32 %3
>       t65: ch = store<(store (s32) into %ir.4)> t0, t8, FrameIndex:i64<3>, undef:i64
>         t6: i32,ch = CopyFromReg t0, Register:i32 %2
>       t68: ch = store<(store (s32) into %ir.3)> t0, t6, FrameIndex:i64<2>, undef:i64
>         t4: i32,ch = CopyFromReg t0, Register:i32 %1
>       t71: ch = store<(store (s32) into %ir.2)> t0, t4, FrameIndex:i64<1>, undef:i64
>     t134: ch = TokenFactor t128, t121:1, t116, t124, t109:1, t131, t98:1, t53, t56, t59, t62, t65, t68, t71
>   t50: ch = AArch64ISD::RET_FLAG t134
> ```
> 
> Post DAG:
> ```
> SelectionDAG has 51 nodes:
>   t0: ch = EntryToken
>   t19: i32,ch = load<(load (s32) from %fixed-stack.1, align 16)> t0, FrameIndex:i64<-1>, undef:i64
>     t2: i64,ch = CopyFromReg t0, Register:i64 %0
>   t51: ch = store<(store (s32) into %ir.a1)> t19:1, t19, t2, undef:i64
>   t98: i32,ch = load<(load (s32) from %fixed-stack.0, align 8)> t51, FrameIndex:i64<-2>, undef:i64
>     t84: i64 = add FrameIndex:i64<-2>, Constant:i64<8>
>   t109: i32,i64,ch = load<(load (s32) from %fixed-stack.0 + 8, align 8), <post-inc>> t51, t84, Constant:i64<8>
>   t120: i32,i64,ch = load<(load (s32)), <post-inc>> t51, t109:1, Constant:i64<8>
>       t126: ch = store<(store (s32) into %ir.a12)> t0, t120, FrameIndex:i64<12>, undef:i64
>       t115: ch = store<(store (s64) into %ir.args)> t0, t120:1, FrameIndex:i64<9>, undef:i64
>       t122: ch = store<(store (s32) into %ir.a11)> t0, t109, FrameIndex:i64<11>, undef:i64
>       t129: ch = store<(store (s32) into %ir.a10)> t0, t98, FrameIndex:i64<10>, undef:i64
>         t16: i32,ch = CopyFromReg t0, Register:i32 %7
>       t53: ch = store<(store (s32) into %ir.8)> t0, t16, FrameIndex:i64<7>, undef:i64
>         t14: i32,ch = CopyFromReg t0, Register:i32 %6
>       t56: ch = store<(store (s32) into %ir.7)> t0, t14, FrameIndex:i64<6>, undef:i64
>         t12: i32,ch = CopyFromReg t0, Register:i32 %5
>       t59: ch = store<(store (s32) into %ir.6)> t0, t12, FrameIndex:i64<5>, undef:i64
>         t10: i32,ch = CopyFromReg t0, Register:i32 %4
>       t62: ch = store<(store (s32) into %ir.5)> t0, t10, FrameIndex:i64<4>, undef:i64
>         t8: i32,ch = CopyFromReg t0, Register:i32 %3
>       t65: ch = store<(store (s32) into %ir.4)> t0, t8, FrameIndex:i64<3>, undef:i64
>         t6: i32,ch = CopyFromReg t0, Register:i32 %2
>       t68: ch = store<(store (s32) into %ir.3)> t0, t6, FrameIndex:i64<2>, undef:i64
>         t4: i32,ch = CopyFromReg t0, Register:i32 %1
>       t71: ch = store<(store (s32) into %ir.2)> t0, t4, FrameIndex:i64<1>, undef:i64
>     t132: ch = TokenFactor t126, t120:2, t115, t122, t109:2, t129, t98:1, t53, t56, t59, t62, t65, t68, t71
>   t50: ch = AArch64ISD::RET_FLAG t132
> ```
> 
> It is hard to argue with this one that the DAG after this change to the combiner is worse, it definitively seems better to me.
It looks like post-inc formation is triggering more on AArch64 as well?

I think it's just something we're going to have to watch out for; if it causes practical issues, we might need to make post-inc formation less aggressive.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124743