[PATCH] D122539: [SelectionDAG][DAGCombiner] Reuse exist node by reassociate

[Phoebe Wang via Phabricator via llvm-commits]

pengfei added inline comments.


================
Comment at: llvm/test/CodeGen/X86/masked-iv-safe.ll:269
 ; CHECK-NEXT:    addq $134217720, %rcx # imm = 0x7FFFFF8
-; CHECK-NEXT:    addq $2040, %rax # imm = 0x7F8
+; CHECK-NEXT:    testq %rax, %rax
 ; CHECK-NEXT:    jne .LBB5_1
----------------
bcl5980 wrote:
> pengfei wrote:
> > This seems regression. The same below.
> Yeah, this change brings some regressions because of two reasons:
> 1. The case you comment, instruction sequence change causes extra compare instructions
> 2. [load/store (add (add x, y), constant)]. Many targets support ld/st with imm offset. If we reassociate instructions, it will miss the address mode pattern match.
> But I think they should be fixed on every backend's isReassocProfitable. I prefer to fix these on individual changes.
We shouldn't commit a patch with any known regressions. An individual fix is fine, but it should be ahead of this one so that we won't bring the regression at any time.


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

https://reviews.llvm.org/D122539