This is an archive of the discontinued LLVM Phabricator instance.

[CVP] Simplify comparisons without constant operand
ClosedPublic

Authored by nikic on Nov 2 2022, 7:05 AM.

Details

Summary

CVP currently only tries to simplify comparisons if there is a constant operand. However, even if both are non-constant, we may be able to determine the result of the comparison based on range information.

IPSCCP is already capable of doing this, but because it runs very early, it may miss some cases.

This has some compile-time cost, but not particularly large: http://llvm-compile-time-tracker.com/compare.php?from=5fe9273c73969791795e5302933abadc9f33f09a&to=4c49e6b0e7d1834dd9e1f29144e915c7279740fb&stat=instructions:u

Diff Detail

Event Timeline

nikic created this revision.Nov 2 2022, 7:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 2 2022, 7:05 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
nikic requested review of this revision.Nov 2 2022, 7:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 2 2022, 7:06 AM
spatel added inline comments.Nov 2 2022, 9:05 AM
llvm/lib/Analysis/LazyValueInfo.cpp
1874

Does something in the callers preclude vector icmp from reaching here? If not, should this use CmpInst::makeCmpResultType()? If yes, then can we assert that LHS/RHS are scalars?

llvm/test/Transforms/CorrelatedValuePropagation/mul.ll
215–216

"add nuw" is all that is needed to fold this and the next test. Do those simpler examples work, or should there be a TODO?

nikic added a subscriber: reames.Nov 2 2022, 9:59 AM
nikic added inline comments.
llvm/test/Transforms/CorrelatedValuePropagation/mul.ll
215–216

It does work with just add nuw (and the nsw on mul gets inferred). TBH, I don't think this test really makes sense. It was added in https://github.com/llvm/llvm-project/commit/f26758486399a542893addd83cd3a1056911d8bd in preparation for handling nuw/nsw flags on mul, so if the flag ends up not mattering, the test probably doesn't test what it was supposed to.

I think what this might have wanted to test is something closer to this? https://alive2.llvm.org/ce/z/BkZtMv

This patch wouldn't be sufficient to handle that, but this patch + mul nsw support should be able to handle it.

cc @reames

nikic updated this revision to Diff 472874.Nov 3 2022, 2:01 AM

Add vector test, use makeCmpResultType().

nikic marked an inline comment as done.Nov 3 2022, 2:03 AM
nikic added inline comments.
llvm/lib/Analysis/LazyValueInfo.cpp
1874

Vector icmps can reach this function, but LVI will always produce overdefined for the operands. That said, it still makes sense to use makeCmpResultType() to make this more future proof, in case LVI gets vector support.

I've also added a vector icmp test case (which currently does not fold).

spatel accepted this revision.Nov 3 2022, 6:16 AM

LGTM

llvm/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp
351

Should update this one to CmpInst::makeCmpResultType() too to future-proof it.

This revision is now accepted and ready to land.Nov 3 2022, 6:16 AM
This revision was landed with ongoing or failed builds.Nov 3 2022, 7:35 AM
This revision was automatically updated to reflect the committed changes.
nikic marked 2 inline comments as done.