[PATCH] D141823: [SCEV] More precise trip multiples

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

In D141823#4057780 <https://reviews.llvm.org/D141823#4057780>, @caojoshua wrote:

> In D141823#4057047 <https://reviews.llvm.org/D141823#4057047>, @nikic wrote:
>
>> In D141823#4056673 <https://reviews.llvm.org/D141823#4056673>, @caojoshua wrote:
>>
>>> In D141823#4055951 <https://reviews.llvm.org/D141823#4055951>, @nikic wrote:
>>>
>>>> Precommit tests please.
>>>
>>> What is precommit test? I've been running llvm-lit llvm/test
>>
>> Commit the new tests with baseline checks (without your patch), and then rebase on top, so it only shows diffs.
>
> Thanks. I've done this for the newest revision. I see why this is preferable, but unless I think I'm missing something, this should be included somewhere in LLVM docs i.e. https://llvm.org/docs/CodeReview.html or https://llvm.org/docs/TestingGuide.html.

I've put up https://reviews.llvm.org/D142441 to update the TestingGuide.

> I have another question. When patches get approved, should owners squash the commits before pushing? Or push two separate commits.

It should be two separate commits. In fact, you can just land the test commit now, without waiting for the patch to be approved.

I believe this patch needs a rebase, because of some recent refactorings.

> My original intent was to use this in getSmallConstantTripMultiples, but it has no effect right now due to change from D110587 <https://reviews.llvm.org/D110587>. For example, if we have backedge count (6 * %N) - 1, the trip count becomes 1 + zext((6 * %N) - 1), and we cannot say that 6 is a multiple of the SCEV. I plan to look further into this separately.

I wonder if it might make sense to address this first? I'm a bit worried that the only test coverage for this functionality we have right now is very indirect, by the effect the multiple has on ranges. It would be great if we could test this functionality directly based on the trip multiple.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141823