This is an archive of the discontinued LLVM Phabricator instance.

Use getLoopLatch in place of isLoopSimplifyForm
ClosedPublic

Authored by trentxintong on Jan 13 2017, 10:15 PM.

Details

Summary

Use getLoopLatch in place of isLoopSimplifyForm. we do not need
to know whether the loop has a preheader nor dedicated exits.

Event Timeline

trentxintong retitled this revision from to Use getLoopLatch in place of isLoopSimplifyForm.
trentxintong updated this object.
trentxintong added reviewers: hfinkel, sanjoy, atrick, mkuper.
trentxintong added a subscriber: llvm-commits.

Remove a redundant getLoopLatch().

sanjoy requested changes to this revision.Jan 13 2017, 10:43 PM
sanjoy edited edge metadata.

Can you please add a test case to unittests/?

This revision now requires changes to proceed.Jan 13 2017, 10:43 PM

Can you please add a test case to unittests/?

will do.

hfinkel edited edge metadata.Jan 14 2017, 8:02 AM

This looks good to me (aside from the fact that, as Sanjoy pointed out, it needs a test case).

trentxintong edited edge metadata.

Add an assertion to check that in case we have a single latch, we actually use that
latch to get the loop id. This is the fast case !.

Add a test case to make sure fast case is exercised and works correctly.

sanjoy requested changes to this revision.Jan 15 2017, 12:36 PM
sanjoy edited edge metadata.

Some comments on the test case.

unittests/Analysis/LoopInfoTest.cpp
20

I'd rather put this inside the LoopInfoTest struct as a public instance variable.

30

Can you ASSERT or assert (up to you which one) that the name of Header is for.cond, just to make sure we're doing what we think we're doing?

52

Make this a static function.

66

This is totally up to you, but I tend to prefer not using explicit "\n" s but instead end lines with a space. That makes the string more readable and it works fine since LLVM is (mostly?) whitespace insensitive.

81

Creating a pass for this seems unnecessary -- why can't you directly call LoopInfo::analyze on the relevant function?

This revision now requires changes to proceed.Jan 15 2017, 12:36 PM
sanjoy added inline comments.Jan 15 2017, 12:38 PM
unittests/Analysis/LoopInfoTest.cpp
85

Also, I'd add a check that the loop is not in loop simplify form, to make sure we're testing the right thing.

trentxintong edited edge metadata.

Address comments.

sanjoy accepted this revision.Jan 15 2017, 1:24 PM
sanjoy edited edge metadata.

lgtm

This revision is now accepted and ready to land.Jan 15 2017, 1:24 PM
This revision was automatically updated to reflect the committed changes.
MatzeB added a subscriber: MatzeB.Jan 17 2017, 10:30 AM

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?

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.

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.

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.

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 makes sense when you're testing an analysis (e.g. does AA think these two pointers may alias or not?). Here the test is for an API that analyses and transformations uses, so to me it is more understandable to test it this way (since you're directly testing what you changed).

However, we could be better about encoding IR in a way that they're easily auto-upgraded, since IMO that's one of the drawbacks of writing unittests/.

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.

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.

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.

Admittedly, I did not think how this should be tested through completely. (I know what I need to test, but as pointed out there are more than 1 way to test this). I sort of followed what @sanjoy suggested. Thanks for raising the concern though.

vasich added a subscriber: vasich.Jun 30 2017, 3:10 AM