Page MenuHomePhabricator

Fix regressions cased by D29862
Needs ReviewPublic

Authored by evstupac on Mar 2 2017, 2:24 PM.

Details

Reviewers
qcolombet
Summary

The patch fix regressions in
Shootout-C++ matrix
and
spec2006/hmmer
caused by D29862.

Diff Detail

Repository
rL LLVM

Event Timeline

evstupac created this revision.Mar 2 2017, 2:24 PM
qcolombet edited edge metadata.Mar 7 2017, 3:33 PM

Hi,

Could you explain what was the problem and how this patch addresses it?

Thanks,
-Quentin

lib/Transforms/Scalar/LoopStrengthReduce.cpp
1080

Unrelated change?

4446

Why don't we have to update Regs anymore?

Could you explain what was the problem and how this patch addresses it?

Sure, here is the case:

Use1 (Address):
{a} + {0,+,1} register num expectation 1 + 1/2
{a,+,1} register num expectation 1
Use2 (Address):
{b} + {0,+,1}
{b,+,1}
Use3 (ICmpZero):
-1024 + {0,+,1}

That way new method will select {a,+1} {b,+,1} and -1024 + {0,+,1} which is not optimal in terms of AddRecExprs (still optimal in terms of RegNum)

In Matrix test there are 32 Address uses like above. The expectation of {a} + {0,+,1} becomes very close 1, but still grater than 1.
The solution is simple - delete formulas in ICmpZero before Address. That way the optimal solution is selected (as {0,+,1} becomes unique).

lib/Transforms/Scalar/LoopStrengthReduce.cpp
4446

This is a kind of unrelated change (as it is redundant initially). We leave only 1 formula in each LSRUse, therfore we already select a solution (or no solution). That way we don't need to RecomputeRegs any more.
If we keep RecomputeRegs here we'll need to sort indexes instead of LSRUses as RecomputeRegs use LSRUse indexes.

evstupac added inline comments.Mar 13 2017, 12:38 PM
lib/Transforms/Scalar/LoopStrengthReduce.cpp
1080

No. This to to make compare function easier (compare enum values):
U1.Kind < U2.Kind

Trying to fix the regressions I've inserted a check which uses Solution generated by NarrowSearchSpaceByDeletingCostlyFormulas() only if it's cost less than cost of regular Solution.
That way NarrowSearchSpaceByDeletingCostlyFormulas() can only decrease final Solution cost. But both matrix and hmmer regression were not resolved.
It turned out that NarrowSearchSpaceByDeletingCostlyFormulas() creates Solution with is better cost (using current cost model) for both cases.
Use of Insns in Cost model helps to resolve the issue.
Right now I see 3 ways to resolve this:

  1. Accept current workaround.
  2. Modify NarrowSearchSpaceByDeletingCostlyFormulas() to rely in Insns instead of Registers.
  3. Change cost model to rely on Insns first and check which solution is better: regular or created by NarrowSearchSpaceByDeletingCostlyFormulas().
lib/Transforms/Scalar/LoopStrengthReduce.cpp
4392

We can sort Uses indexes here instead of sorting Uses. That way we'll avoid change on line 4435.

How do you think which way we should follow?
Or we can leave regression as is as Solution Cost is better and therefore it is not a problem of newly implemented NarrowSearchSpaceByDeletingCostlyFormulas()?