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

[Guozhi Wei via Phabricator via llvm-commits]

Carrot added inline comments.


================
Comment at: llvm/test/CodeGen/X86/reassociate-add.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mtriple=x86_64-unknown-unknown -mcpu=x86-64 < %s | FileCheck %s
+
----------------
RKSimon wrote:
> Drop -mcpu=x86-64
> 
> Pre-commit these test with current codegen to trunk and rebase to show the patch diffs.
Test case has been sent out as https://reviews.llvm.org/D136501.

 -mcpu=x86-64 is required because in MachineCombiner the reassociation is performed either the new sequence reduce latency, or  function doSubstitute returns true. In its implementation, if -mcpu is not specified, the target will not have a valid schedule model, and then doSubstitute returns true, and reassociation is performed.

```
bool MachineCombiner::doSubstitute(unsigned NewSize, unsigned OldSize,
                                   bool OptForSize) {
  if (OptForSize && (NewSize < OldSize))
    return true;
  if (!TSchedModel.hasInstrSchedModelOrItineraries())
    return true;
  return false;
}
```


================
Comment at: llvm/test/CodeGen/X86/xmulo.ll:1871
 ; WIN32-NEXT:    setne %al
-; WIN32-NEXT:    addl $12, %esp
+; WIN32-NEXT:    addl $16, %esp
 ; WIN32-NEXT:    popl %esi
----------------
RKSimon wrote:
> Annoying - normally I'd expect reassociation to help us reduce stack use
Agree it is annoying.
But I think the impact of reassociation to register allocation is quite randomly, it depends on if the operands can be killed by the first instruction, and later be scheduled earlier to shorten the operands' live range.

In this specific case, although it increases stack use, it reduces register spill/reload instructions.
```
$ git diff origin/main xmulo.ll | grep "+;" | wc
    104     518    3715
$ git diff origin/main xmulo.ll | grep "\-;" | wc
    108     542    3846
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136396