This is an archive of the discontinued LLVM Phabricator instance.

Unroll restructure
ClosedPublic

Authored by evstupac on Jun 24 2016, 7:01 PM.

Details

Summary

Fix potential overflow.
Use BEInsns instead of constant 2.
Add a function checking if LoopSize meet unroll preferences.

make check passed
spec2000 - build same for all

Diff Detail

Event Timeline

evstupac updated this revision to Diff 61877.Jun 24 2016, 7:01 PM
evstupac retitled this revision from to Unroll restructure.
evstupac updated this object.
evstupac added reviewers: mzolotukhin, sanjoy.
evstupac set the repository for this revision to rL LLVM.
evstupac added subscribers: llvm-commits, mehdi_amini.
mzolotukhin edited edge metadata.Jun 27 2016, 8:14 AM

Hi,

Please find a couple of question/remarks below:

Fix potential overflow.

Is it possible to compose a test for it?

Use BEInsns instead of constant 2.

Why do we need it? At least, we can make it const I think.

Thanks,
Michael

lib/Transforms/Scalar/LoopUnrollPass.cpp
699 ↗(On Diff #61877)

s/isLoopSizeMeetUP/doesLoopSizeMeetUP/

Also, what about something like isLoopSizeLessThanThreshold?

704–711 ↗(On Diff #61877)

Why can't we just pass PragmaUnrollThreshold/UP.PartialThreshold/UP.Threshold instead of ThresholdType TT?

Fix potential overflow.

Is it possible to compose a test for it?

That is regarding 32 bits multiplication:

(LoopSize - BEInsns) * UP.Count

I don't think we should make a test here. However I think it is possible.

Use BEInsns instead of constant 2.

Why do we need it? At least, we can make it const I think.

That was comment from hfinkel in D19553.
BEInsns makes code easier to read. As for const value, yes it could be const now, but some architectures may have another value. Moreover loop analysis can point us to some other instruction that could be optimized after unroll. Tat way we'll be able to increase BEInsns.

lib/Transforms/Scalar/LoopUnrollPass.cpp
699 ↗(On Diff #61877)

isLoopSizeLessThanThreshold

This function check if LoopSize less than threshold but after unrolling. That way Count and BEInsns make differ.

704–711 ↗(On Diff #61877)

In most cases Threshold are in UP, so we need to pass only 2 parameters which is less. Also passing structure reference and its field value as different parameters looks strange.

mzolotukhin added inline comments.Aug 30 2016, 3:29 PM
lib/Transforms/Scalar/LoopUnrollPass.cpp
704–711 ↗(On Diff #61877)

I'd rather create a function that would compute (LoopSize - UP.BEInsns) * UP.Count + UP.BEInsns. I don't see a value in passing a threshold and comparing against it inside the function. It causes code duplication inside and requires an additional parameter.

evstupac updated this revision to Diff 70640.Sep 7 2016, 7:50 PM
evstupac edited edge metadata.

Changed according to review comments.

mzolotukhin accepted this revision.Sep 27 2016, 6:46 PM
mzolotukhin edited edge metadata.

LGTM with minor nits.

Thanks,
Michael

lib/Transforms/Scalar/LoopUnrollPass.cpp
127 ↗(On Diff #70640)

It would make sense to add the initialization to getUnrollingPreferences in BasicTTIImpl.h.

694 ↗(On Diff #70640)

Please use verbs for function names and start them from lower-case.

743 ↗(On Diff #70640)

Is this unused now?

This revision is now accepted and ready to land.Sep 27 2016, 6:46 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 7 2019, 6:40 AM