Page MenuHomePhabricator

[SCEV] Return zero from computeConstantDifference(X, X)
ClosedPublic

Authored by n.bozhenov on Jul 30 2019, 1:06 PM.

Details

Summary

Without this patch computeConstantDifference returns None for cases like
these:

computeConstantDifference(%x, %x)
computeConstantDifference({%x,+,16}, {%x,+,16})

Diff Detail

Repository
rL LLVM

Event Timeline

n.bozhenov created this revision.Jul 30 2019, 1:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 30 2019, 1:06 PM
Herald added a subscriber: hiraditya. · View Herald Transcript

Test?

I agree that it would be nice to have a test for this, but I failed to find a way to test it. I would appreciate it if you could suggest how I could test the result of computeConstantDifference method. Please, keep in mind that the method is private.

Alternatively, we could commit this patch without a dedicated test, as the code is trivial. This piece of code is executed multiple times when running make check (about 400 lit tests). It doesn't change the observed behavior, though.

We use computeConstantDifference in some downstream code, and I was surprised to find out that it fails to compute (X - X) difference, unless X is a SCEVAddExpr.

sanjoy requested changes to this revision.Jul 30 2019, 3:01 PM

We use computeConstantDifference in some downstream code, and I was surprised to find out that it fails to compute (X - X) difference, unless X is a SCEVAddExpr.

Can you use getMinusSCEV instead (which should DTRT)?

This looks like a small change, having many such cases (especially without unit tests) makes it difficult to refactor code confidently.

This revision now requires changes to proceed.Jul 30 2019, 3:01 PM

We use computeConstantDifference in some downstream code, and I was surprised to find out that it fails to compute (X - X) difference, unless X is a SCEVAddExpr.

Can you use getMinusSCEV instead (which should DTRT)?

This looks like a small change, having many such cases (especially without unit tests) makes it difficult to refactor code confidently.

Hi Sanjoy,

What I need is to check if two arbitrary SCEV expressions are constant distance apart or not, and compute that difference. That's exactly what computeConstantDifference does (except for the (%x - %x) case). I would be ok to use getMinusSCEV, but it does not do the right thing unfortunately. Given {%x, +, 1}<loop1> and {%x, +, 1}<loop2>, getMinusSCEV crashes instead of returning SCEVCouldNotCompute. And I am not aware of any existing API that checks whether two SCEVs are safe to subtract or not.

We use computeConstantDifference in some downstream code, and I was surprised to find out that it fails to compute (X - X) difference, unless X is a SCEVAddExpr.

Can you use getMinusSCEV instead (which should DTRT)?

This looks like a small change, having many such cases (especially without unit tests) makes it difficult to refactor code confidently.

Hi Sanjoy,

What I need is to check if two arbitrary SCEV expressions are constant distance apart or not, and compute that difference. That's exactly what computeConstantDifference does (except for the (%x - %x) case). I would be ok to use getMinusSCEV, but it does not do the right thing unfortunately. Given {%x, +, 1}<loop1> and {%x, +, 1}<loop2>, getMinusSCEV crashes instead of returning SCEVCouldNotCompute. And I am not aware of any existing API that checks whether two SCEVs are safe to subtract or not.

That already sounds like a test.

fhahn added a subscriber: fhahn.Jul 31 2019, 1:35 AM

IMO it would make sense to add isolated unit tests to llvm/unittests/Analysis/ScalarEvolutionTest.cpp for such behavior. It should be easy to test there. I *think* there should be no harm in making ScalarEvolutionTest a friend of ScalarEvolution to test private members.

n.bozhenov updated this revision to Diff 212944.Aug 1 2019, 5:59 PM

I am afraid implementing a test through friendship would be pretty cumbersome.
ScalarEvolutionsTest performs checks inside lambdas, while the friendship does
not propagate inside lambdas.

A more straightforward approach would be to make computeConstantDifference
public. Please, check the updated patch with a unit test.

sanjoy requested changes to this revision.Aug 4 2019, 10:28 AM

Let's make this a friend function, I'm already not super happy with how wide SCEV's API is. You can make the diff lambda a member function of ScalarEvolutionsTest to make this work.

Other than that lgtm.

This revision now requires changes to proceed.Aug 4 2019, 10:28 AM
n.bozhenov updated this revision to Diff 213381.Aug 5 2019, 9:19 AM

Thanks for the suggestion, Sanjoy. Please, review the updated patch.

sanjoy accepted this revision.Aug 6 2019, 7:31 PM

LGTM

This revision is now accepted and ready to land.Aug 6 2019, 7:31 PM
This revision was automatically updated to reflect the committed changes.