This is an archive of the discontinued LLVM Phabricator instance.

[LoopUnroll] Keep UP.Count when considering complete unroll
AbandonedPublic

Authored by zzheng on Dec 18 2016, 7:20 PM.

Details

Summary

r286389 updates UP.Count unconditionally when considering complete unroll of loop with compile-time known TripCount.
This can overwrite UP.Count retrieved by getUnrollingPreferences().

Diff Detail

Repository
rL LLVM

Event Timeline

zzheng updated this revision to Diff 81912.Dec 18 2016, 7:20 PM
zzheng retitled this revision from to [LoopUnroll] Keep UP.Count when considering complete unroll.
zzheng updated this object.
zzheng added reviewers: evstupac, mzolotukhin, zinob.
zzheng set the repository for this revision to rL LLVM.
zzheng added a subscriber: llvm-commits.
evstupac edited edge metadata.Dec 20 2016, 1:29 PM

I agree that r286389 have changed behavior.
However to prevent this in future would you mind to add a test case?

However to prevent this in future would you mind to add a test case?

This issue was found in our in-house branch with custom TTI::getUnrollingPreferences()... not easy to produce a test case.

evstupac added inline comments.Jan 5 2017, 6:01 PM
lib/Transforms/Scalar/LoopUnrollPass.cpp
785

Will it work if we put UP.Count change under FullUnrollTripCount condition?
If we leave the change as is it could happen that for full unroll UP.Count is less than MaxTripCount. Which is not ok.
Also could you please add context to the patch:
svn diff --diff-cmd=diff -x -U999999

zzheng updated this revision to Diff 83759.Jan 9 2017, 5:58 PM
zzheng edited edge metadata.

Moved UP.Count = FullUnrollTripCount under condition as suggested by Evgeny.

haicheng added inline comments.Jan 9 2017, 7:05 PM
lib/Transforms/Scalar/LoopUnrollPass.cpp
761

I think you need to set UP.Count in this situation, too.

haicheng added inline comments.Jan 9 2017, 7:08 PM
lib/Transforms/Scalar/LoopUnrollPass.cpp
761

Sorry, never mind.

This issue was found in our in-house branch with custom TTI::getUnrollingPreferences()... not easy to produce a test case.

It looks like no one from trunk targets has UP.Count set in Unroll Preferences. So to add a test we'll need to add an option setting UP.Count default.

LGTM if all tests are passed.

evstupac accepted this revision.Jan 10 2017, 12:12 PM
evstupac edited edge metadata.
This revision is now accepted and ready to land.Jan 10 2017, 12:12 PM
zzheng abandoned this revision.Jun 6 2019, 6:23 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 6 2019, 6:23 PM
Herald added a subscriber: dmgreen. · View Herald Transcript