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

Florian Hahn via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Aug 2 11:57:21 PDT 2019


fhahn added a comment.

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

> Sorry about the delayed response.


No worries, thank you very much for taking the time to take a look!

> So currently, this is essentially a three-patch series; D60581 <https://reviews.llvm.org/D60581>, D60582 <https://reviews.llvm.org/D60582>, and D61314 <https://reviews.llvm.org/D61314>.  D61314 <https://reviews.llvm.org/D61314> proposes to get rid of "forced" constants, and use plain constants instead.  I'm a little confused how that works; are you just getting rid of the assertion that a constant can't have two different values?

Sorry for not better organizing things. I wanted to make sure the overall direction makes sense (remove forcedconstant, use integer ranges) before re-arranging things. I'll rebase things and bring them in order and link them in Phab.

The idea here is to use the fact that integer constants are represented via a range, so they can have multiple values, and SCCP must take care of updating dependent instructions, if a range changes. This should allow us to get the same (or slightly more precise) behavior as forcedconstant: assume an arbitrary value and be sound when we later discover a different constant value for the same value.

When we used to assume a value through forcedconstant, we now just create a singleton integer range. Should we later discover a different constant value, instead of going to overdefined, we extend the range to include the new value and update the dependent instructions. I think for integer constants we can construct ranges for, that should work out fine.

For constants we cannot construct ranges for (e.g. floats or constants without specific integer value), we should still have the assertion (ValueLatticeElement should have the same assertions as the removed code from the ValueLattice in this file). Thinking about it again now, there might be some scenarios where we might hit it. It did not surface on a bootstrap build of LLVM with LTO, but I'll see if I can construct one.

> 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?

For operations where the original lattice value is a non-singleton range and the result of an operation is different (larger) range, we go to overdefined (around line 913). That should be the most conservative form of widening and should handle binary ops in loops, because we never extend a range more than once before going to overdefined. With might be able to use slightly more involved widening approaches later on.

We need this for all operators, I'll double check to make sure we do that for all operators.

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

Understood, this change is quite disruptive. Is there anything in particular you are worried about/ anything I could do to sort out any issues there?


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

https://reviews.llvm.org/D60582





More information about the llvm-commits mailing list