[PATCH] D136396: [X86] Enable reassociation for ADD instructions

Craig Topper via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Oct 21 11:15:24 PDT 2022


craig.topper added inline comments.


================
Comment at: llvm/test/CodeGen/X86/reassociate-add.ll:4
+
+; This file checks the reassociation of ADD instruction.
+; The two ADD instructions add v0,v1,t2 together. t2 has a long dependence
----------------
Carrot wrote:
> spatel wrote:
> > RKSimon wrote:
> > > pengfei wrote:
> > > > No idea if we intended to not do reassociation in ADD instructions. 
> > > > Is there problem when unexpected overflow/underflow may be generated during reassociation?
> > > Not that I can think of - EFLAGS will be the same and we already handles the $dst=$src0 constraint
> > I was working on this a long time ago (~2015), so it's hard to remember exactly, but I don't think there was a fundamental reason to exclude integer ADD. 
> > 
> > It just seemed like it did not have much potential gain with the limited register set and could interfere with other transforms like LEA formation. 
> > 
> > If there's evidence that this improves something (and doesn't cause regressions), then it should be ok.
> EFLAGS may be different. Suppose we are adding three bytes, 250 + 10 + 10. 
> 
>   - If we add them in the order (250 + 10) + 10, the first ADD generates carry/overflow flags, the second ADD doesn't generate carry/overflow.
> 
>   - If we add them in the order (10 + 10) + 250, the first ADD doesn't generates carry/overflow flags, the second ADD generates these flags.
> 
> So we need to check if the definition of EFLAGS is dead.
> 
> Maybe this is the reason @spatel didn't add ADD instructions in 2015.
> 
> Thank @pengfei for the reminds.
> 
Scalar MUL would have the same EFLAGs issue. So hopefully we already solved it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136396



More information about the llvm-commits mailing list