[PATCH] D28724: Use getLoopLatch in place of isLoopSimplifyForm

Matthias Braun via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Jan 17 11:10:36 PST 2017


MatzeB added a comment.

In https://reviews.llvm.org/D28724#648438, @trentxintong wrote:

> In https://reviews.llvm.org/D28724#648408, @MatzeB wrote:
>
> > In https://reviews.llvm.org/D28724#648407, @trentxintong wrote:
> >
> > > In https://reviews.llvm.org/D28724#648400, @MatzeB wrote:
> > >
> > > > In https://reviews.llvm.org/D28724#648397, @MatzeB wrote:
> > > >
> > > > > Why was this implemented as a unittest? Wouldn't this work just as good by inserting some debug prints and then using lit+opt+FileCheck to test?
> > > >
> > > >
> > > > Admittedly the same is true for many of the other tests in unittests/Analysis. Most of the times I just don't see the benefits of using unittests over lit+FileCheck, they are usually a lot harder to understand and debug.
> > >
> > >
> > > One problem I can see with LIT test here is that getLoopID() is not a transformation by itself, it depends on other transformations to call/use it. Would not using a LIT test for that tranformation making the testing a little bit more fragile/unpredictable, i.e. what if the transformation later is changed not to call this API or call it in a different way.  Testing this way is a bit more specific I feel.
> >
> >
> > You can just request a single analysis from opt without running any transformation. It should work similar to this `opt -mypass -debug-only=mypass input.ll` if you use DEBUG(dbgs() << ) statements, alternatively there is the -analyze flag to opt, which ends up calling MyAnalysisPass::print().
> >
> > There are number of tests in test/Analysis that work this way.
>
>
> I think that would work for LoopInfo, with some additional debug printing. One problem/question I have is whether its a good idea to call getLoopID()/setLoopID() in ::print(), should not print() be primarily used to dump existing information, than testing some APIs in the analysis.


If course print() should not change any information.
At a first glance this looked to me like you just test the result of an anlysis. But indeed if you need to test actual interaction with an API the unittest is the right tool.


Repository:
  rL LLVM

https://reviews.llvm.org/D28724





More information about the llvm-commits mailing list