[PATCH] D60582: [IPSCCP] Add general integer range support.

[Florian Hahn via Phabricator via llvm-commits]

fhahn updated this revision to Diff 235423.
fhahn added a comment.

ping :)

I finally found some time to rebase this and I also tried to reduce the diff as far as possible!

This patch now just replaces SCCP's LatticeVal with ValueLatticeElement, without making additional use of constant ranges  (besides for function arguments) just yet. As follow-up, I'll add patches making use of range information for various kinds of instructions. This patch itself should be a NFC (compiling test-suite & SPEC produces equal binaries with and without this change for -O3) and uses the following helpers to preserve the current behavior when constant ranges are used for integers:

- isConstant(LV): returns true when LV is either a constant or a constant range with a single element. This should return true in the same cases where LV.isConstant() returned true previously.
- getConstant(LV): returns a constant if LV is either a constant or a constant range with a single element. This should return a constant in the same cases as LV.getConstant() previously.
- getConstantInt(LV): same as getConstant, but additionally casted to ConstantInt.



In D60582#1612809 <https://reviews.llvm.org/D60582#1612809>, @fhahn wrote:

> In D60582#1611337 <https://reviews.llvm.org/D60582#1611337>, @efriedma wrote:
>
> >
>




>> How do constant ranges work with loops?  It seems like under a naive implementation, an induction variable would start a 0, then grow to 0-1, then grow to 0-2, etc., until it covers the full integer range.  That clearly doesn't happen, since it would take forever, but I'm not sure I understand the limiting factor.  There's a comment in visitBinaryOperator about this, but does that cover all possible loops?

I've moved the widening to mergeInValue. This guarantees we set/extend a range at most once and go to overdefined otherwise. That should cover all loops. The one exception is when merging function argument values, but that should be fine, because the concrete argument value ranges can only extended at most once.

> 
> 
>> I'm a little wary of adding complexity to the lattice given the issues we currently have reasoning about the semantics of "undef"/ResolveUndefsIn.

The updated patches break things down much better I think and allow it to more carefully review the changes. I don't think there's an impact on ResolveUndefsIn at the moment, although begin more powerful might expose additional issues. But I think we could look at the issues as they appear.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60582/new/

https://reviews.llvm.org/D60582

Files:
  llvm/include/llvm/Analysis/ValueLattice.h
  llvm/lib/Transforms/Scalar/SCCP.cpp
  llvm/test/Transforms/SCCP/ip-constant-ranges.ll
  llvm/test/Transforms/SCCP/ipsccp-range-crashes.ll
  llvm/test/Transforms/SCCP/ipsccp-range-structs.ll

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D60582.235423.patch
Type: text/x-patch
Size: 36667 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20191227/e69a48b0/attachment.bin>