Page MenuHomePhabricator

[SCEV] Handle `less` predicates for FoundPred = NE
ClosedPublic

Authored by mkazantsev on Sep 18 2020, 1:47 AM.

Details

Summary

Currently these predicates are ignored, yet their handling is
pretty simple. I could not find a single test where it would
actually change something, but it's only because isImpliedCondOperands
is not smart enough to prove it further on. Yet the situation when
we come there with less predicate is pretty common.

Diff Detail

Event Timeline

mkazantsev created this revision.Sep 18 2020, 1:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 18 2020, 1:47 AM
mkazantsev requested review of this revision.Sep 18 2020, 1:47 AM
fhahn added a comment.Sep 18 2020, 1:52 AM

Would it be worth adding a C++ unit test for those cases that just passes in suitable predicates + SCEV expression?

Yes, I will try to construct such test.

mkazantsev abandoned this revision.Sep 21 2020, 1:14 AM

Actually I'm not sure we need this. I could not construct a test where it would succeed.

mkazantsev reclaimed this revision.Sep 22 2020, 12:08 AM

It seems that there is no way that SCEV can now both make this query and return true for it. I'm currently building a chain of patch that will fix the query responds. Hopefully with them we will be able to construct a test for it.

Or can we just merge it basing on obviousness of this logic?

fhahn added a comment.Sep 22 2020, 2:28 AM

It seems that there is no way that SCEV can now both make this query and return true for it. I'm currently building a chain of patch that will fix the query responds. Hopefully with them we will be able to construct a test for it.

Or can we just merge it basing on obviousness of this logic?

Sure that is fine, if there will be patches to make use of it in the near future.

LGTM, thanks.

llvm/lib/Analysis/ScalarEvolution.cpp
9763

nit: Might be good to mention both the LE & LT variants and say that both LHS <= RHS and LHS < RHS are handled in the same way as RHS >= LHS and RHS > LHS respectively.

fhahn accepted this revision.Sep 22 2020, 2:28 AM
This revision is now accepted and ready to land.Sep 22 2020, 2:28 AM
mkazantsev added inline comments.Sep 22 2020, 4:10 AM
llvm/lib/Analysis/ScalarEvolution.cpp
9763

Ok.

Thanks Florian! One example of where it helps is ULT check in test of D88087 (fails without this patch).

This revision was automatically updated to reflect the committed changes.