[all-commits] [llvm/llvm-project] 3ba83f: [NFC][InstCombine] Add tests for PHI CSE

[Roman Lebedev via All-commits]

  Branch: refs/heads/master
  Home:   https://github.com/llvm/llvm-project
  Commit: 3ba83f2d29e5103b864a5a2f957bd10be73d9663
      https://github.com/llvm/llvm-project/commit/3ba83f2d29e5103b864a5a2f957bd10be73d9663
  Author: Roman Lebedev <lebedev.ri at gmail.com>
  Date:   2020-08-29 (Sat, 29 Aug 2020)

  Changed paths:
    A llvm/test/Transforms/InstCombine/phi-cse.ll

  Log Message:
  -----------
  [NFC][InstCombine] Add tests for PHI CSE

As discussed in https://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20200824/824235.html
even though it seems worthwhile doing so in InstSimplify,
we really can't do that there, because the other PHI wouldn't be
def-reachable from the original PHI.

So ignoring whether or not EarlyCSE should also know to do this,
InstCombine is the place.


  Commit: aea042e978bf7b7af95336916750b4f737044291
      https://github.com/llvm/llvm-project/commit/aea042e978bf7b7af95336916750b4f737044291
  Author: Roman Lebedev <lebedev.ri at gmail.com>
  Date:   2020-08-29 (Sat, 29 Aug 2020)

  Changed paths:
    M llvm/test/Transforms/InstSimplify/phi-cse.ll

  Log Message:
  -----------
  [NFC][InstSimplify] Add a note to PHI CSE tests that they are all negative tests

As discussed in https://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20200824/824235.html
even though it seems worthwhile doing so in InstSimplify,
we really can't do that there, because the other PHI wouldn't be
def-reachable from the original PHI.


  Commit: 3e69871ab5a66fb55913a2a2f5e7f5b42899a4c9
      https://github.com/llvm/llvm-project/commit/3e69871ab5a66fb55913a2a2f5e7f5b42899a4c9
  Author: Roman Lebedev <lebedev.ri at gmail.com>
  Date:   2020-08-29 (Sat, 29 Aug 2020)

  Changed paths:
    M llvm/lib/IR/Instruction.cpp
    M llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp
    M llvm/test/Transforms/InstCombine/merging-multiple-stores-into-successor.ll
    M llvm/test/Transforms/InstCombine/phi-aware-aggregate-reconstruction.ll
    M llvm/test/Transforms/InstCombine/phi-cse.ll
    M llvm/test/Transforms/InstCombine/phi-equal-incoming-pointers.ll
    M llvm/test/Transforms/InstCombine/select.ll
    M llvm/test/Transforms/LoopVectorize/reduction.ll

  Log Message:
  -----------
  [InstCombine] Take 2: Perform trivial PHI CSE

The original take was 6102310d814ad73eab60a88b21dd70874f7a056f,
which taught InstSimplify to do that, which seemed better at time,
since we got EarlyCSE support for free.

However, it was proven that we can not do that there,
the simplified-to PHI would not be reachable from the original PHI,
and that is not something InstSimplify is allowed to do,
as noted in the commit ed90f15efb40d26b5d3ead3bb8e9e284218e0186
that reverted it :
> It appears to cause compilation non-determinism and caused stage3 mismatches.

However InstCombine already does many different optimizations,
so it should be a safe place to do it here.

Note that we still can't just compare incoming values ranges,
because there is no guarantee that these PHI's we'd simplify to
were already re-visited and sorted.
However coming up with a test is problematic.

Effects on vanilla llvm test-suite + RawSpeed:
```
| statistic name                                     | baseline  | proposed  |      Δ |        % |      |%| |
|----------------------------------------------------|-----------|-----------|-------:|---------:|---------:|
| instcombine.NumPHICSEs                             | 0         | 22228     |  22228 |    0.00% |    0.00% |
| asm-printer.EmittedInsts                           | 7942329   | 7942456   |    127 |    0.00% |    0.00% |
| assembler.ObjectBytes                              | 254295632 | 254313792 |  18160 |    0.01% |    0.01% |
| early-cse.NumCSE                                   | 2183283   | 2183272   |    -11 |    0.00% |    0.00% |
| early-cse.NumSimplify                              | 550105    | 541842    |  -8263 |   -1.50% |    1.50% |
| instcombine.NumAggregateReconstructionsSimplified  | 73        | 4506      |   4433 | 6072.60% | 6072.60% |
| instcombine.NumCombined                            | 3640311   | 3666911   |  26600 |    0.73% |    0.73% |
| instcombine.NumDeadInst                            | 1778204   | 1783318   |   5114 |    0.29% |    0.29% |
| instcount.NumCallInst                              | 1758395   | 1758804   |    409 |    0.02% |    0.02% |
| instcount.NumInvokeInst                            | 59478     | 59502     |     24 |    0.04% |    0.04% |
| instcount.NumPHIInst                               | 330557    | 330549    |     -8 |    0.00% |    0.00% |
| instcount.TotalBlocks                              | 1077138   | 1077221   |     83 |    0.01% |    0.01% |
| instcount.TotalFuncs                               | 101442    | 101441    |     -1 |    0.00% |    0.00% |
| instcount.TotalInsts                               | 8831946   | 8832611   |    665 |    0.01% |    0.01% |
| simplifycfg.NumInvokes                             | 4300      | 4410      |    110 |    2.56% |    2.56% |
| simplifycfg.NumSimpl                               | 1019813   | 999740    | -20073 |   -1.97% |    1.97% |
```
So it fires ~22k times, which is less than ~24k the take 1 did.
It allows foldAggregateConstructionIntoAggregateReuse() to actually work
after PHI-of-extractvalue folds did their thing. Previously SimplifyCFG
would have done this PHI CSE, of all places. Additionally, allows some
more `invoke`->`call` folds to happen (+110, +2.56%).

All in all, expectedly, this catches less things overall,
but all the motivational cases are still caught, so all good.


Compare: https://github.com/llvm/llvm-project/compare/fc2dac4116df...3e69871ab5a6