This is an archive of the discontinued LLVM Phabricator instance.

Factor out UnrollAnalyzer to Analysis, and add unit tests for it.
ClosedPublic

Authored by mzolotukhin on Jan 26 2016, 8:50 PM.

Details

Summary

Unrolling Analyzer is already pretty complicated, and it becomes harder and harder to exercise it with usual IR tests, as with them we can only check the final decision: whether the loop is unrolled or not. This change factors this framework out from LoopUnrollPass to analyses, which allows to use unit tests.
The change itself is supposed to be NFC, except adding a couple of tests.

I plan to add more tests as I add new functionality and find/fix bugs.

Diff Detail

Repository
rL LLVM

Event Timeline

mzolotukhin retitled this revision from to Factor out UnrollAnalyzer to Analysis, and add unit tests for it..
mzolotukhin updated this object.
mzolotukhin added reviewers: chandlerc, hfinkel.
mzolotukhin added a subscriber: llvm-commits.
sanjoy requested changes to this revision.Jan 26 2016, 10:04 PM
sanjoy added a reviewer: sanjoy.

I think this change requires some reorganization before it can go in. Please take a look at http://llvm.org/docs/CodingStandards.html

include/llvm/Analysis/LoopUnrollAnalyzer.h
6 ↗(On Diff #46098)

This is very unlike headers in the rest of LLVM -- have you looked at other headers in this directory? For instance:

  • Headers should not open namespaces
  • License header is missing
  • Function bodies (e.g. visitBinaryOperator) should be in the cpp file
25 ↗(On Diff #46098)

This should be in namespace llvm.

I'd say it is better to not even expose this as an InstVisitor but keep that an implementation detail; but that's debatable.

This revision now requires changes to proceed.Jan 26 2016, 10:04 PM

Thanks for the feedback, Sanjoy! Yep, I haven't done such refactorings in llvm before, so I really appreciate such feedback.
I'll update the patch shortly.

Thanks,
Michael

zzheng added a subscriber: zzheng.Jan 27 2016, 12:16 PM
mzolotukhin edited edge metadata.
  • Better follow coding standards.
mzolotukhin marked 2 inline comments as done.Jan 29 2016, 11:58 AM

Hi Sanjoy,

I updated the patch, please take a look. As for not exposing it as InstVisitor: my intention to move this functionality out is just to enable unit-testing for it. And unit-testing in this case is much easier, as we can actually pin-point interesting cases (e.g. given just two instructions A and B, if we know that A is simplified to C, do we simplify B?). However, I'm happy to reconsider this if there are arguments against it.

Thanks,
Michael

sanjoy accepted this revision.Feb 5 2016, 2:18 PM
sanjoy edited edge metadata.

LGTM with minor suggestions, sorry for the delay. I only casually skimmed through the logic, since this is supposed to be an NFC.

lib/Analysis/LoopUnrollAnalyzer.cpp
19 ↗(On Diff #46331)

Minor: usually .cpp files tend to using namespace llvm; instead of this.

unittests/Analysis/UnrollAnalyzer.cpp
22 ↗(On Diff #46331)

Minor: I'd just make these static and remove the anonymous namespace. Instead, the anonymous namespace needs to be around struct UnrollAnalyzerTest.

This revision is now accepted and ready to land.Feb 5 2016, 2:18 PM
This revision was automatically updated to reflect the committed changes.