This is an archive of the discontinued LLVM Phabricator instance.

[SCEV] Disable canonical expansion for nested recurrences.
ClosedPublic

Authored by ebrevnov on Jul 25 2019, 5:37 AM.

Details

Summary

This is a continuation of https://reviews.llvm.org/D62563. Essentially I took latest change proposed by Artur and tried to address Philip's concern "Does this mean that evaluateAtIteration may return an incorrect result? If so, I'd very much like to see an assert which trips on that usage."

Diff Detail

Repository
rL LLVM

Event Timeline

ebrevnov created this revision.Jul 25 2019, 5:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 25 2019, 5:37 AM

The original patch introduces a function minIterationWidthForEvaluateAtIteration. This way it exposes the limitation of evaluateAtIteration through the API and makes it possible for the user to choose the proper It width (e.g. like I did in the first revision https://reviews.llvm.org/D62563?vs=on&id=201795). What was the reason to remove it?

include/llvm/Analysis/ScalarEvolutionExpressions.h
347–349 ↗(On Diff #211728)

Because you removed minIterationWidthForEvaluateAtIteration from the API, now the description on evaluateAtIteration is very vague. I'd prefer to specify this limitation more specifically.

lib/Analysis/ScalarEvolution.cpp
1208–1212 ↗(On Diff #211728)

Any reason to express this check in this way and not as a comparison of CalculationBits with It bitwidth?

11670 ↗(On Diff #211728)

I don't think that it makes much sense to ask loop disposition of scCouldNotCompute. I think the caller of this API should handle scCouldNotCompute case instead.

ebrevnov updated this revision to Diff 213570.Aug 6 2019, 3:45 AM
This comment was removed by ebrevnov.
ebrevnov updated this revision to Diff 213574.Aug 6 2019, 3:53 AM
This comment was removed by ebrevnov.
ebrevnov updated this revision to Diff 213580.Aug 6 2019, 4:12 AM

Splitting exitsting patch to two parts. The first one is an actual fix for the problem. The second one is an enhancement to evaluateAtIteration which is going to be reviewd and committed separately.

ebrevnov updated this revision to Diff 213789.Aug 6 2019, 9:31 PM
This comment was removed by ebrevnov.
ebrevnov updated this revision to Diff 213790.Aug 6 2019, 9:33 PM
apilipenko accepted this revision.Sep 20 2019, 6:27 PM
This revision is now accepted and ready to land.Sep 20 2019, 6:27 PM
This revision was automatically updated to reflect the committed changes.