[llvm] c245d3e - [ValueLattice] Steal bits from Tag to track range extensions (NFC).

[Florian Hahn via llvm-commits]

Author: Florian Hahn
Date: 2020-04-17T15:38:23+01:00
New Revision: c245d3e033a58582475c3d749085f47a9ab0dda1

URL: https://github.com/llvm/llvm-project/commit/c245d3e033a58582475c3d749085f47a9ab0dda1
DIFF: https://github.com/llvm/llvm-project/commit/c245d3e033a58582475c3d749085f47a9ab0dda1.diff

LOG: [ValueLattice] Steal bits from Tag to track range extensions (NFC).

Users of ValueLatticeElement currently have to ensure constant ranges
are not extended indefinitely. For example, in SCCP, mergeIn goes to
overdefined if a constantrange value is repeatedly merged with larger
constantranges. This is a simple form of widening.

In some cases, this leads to an unnecessary loss of information and
things can be improved by allowing a small number of extensions in the
hope that a fixed point is reached after a small number of steps.

To make better decisions about widening, it is helpful to keep track of
the number of range extensions. That state is tied directly to a
concrete ValueLatticeElement and some unused bits in the class can be
used. The current patch preserves the existing behavior by default:
CheckWiden defaults to false and if CheckWiden is true, a single change
to the range is allowed.

Follow-up patches will slightly increase the threshold for widening.

Reviewers: efriedma, davide, mssimpso

Reviewed By: efriedma

Differential Revision: https://reviews.llvm.org/D78145

Added: 
    

Modified: 
    llvm/include/llvm/Analysis/ValueLattice.h
    llvm/lib/Transforms/Scalar/SCCP.cpp

Removed: 
    


################################################################################
diff  --git a/llvm/include/llvm/Analysis/ValueLattice.h b/llvm/include/llvm/Analysis/ValueLattice.h
index 1c4683857f98..6f054fd1ffc8 100644
--- a/llvm/include/llvm/Analysis/ValueLattice.h
+++ b/llvm/include/llvm/Analysis/ValueLattice.h
@@ -75,7 +75,9 @@ class ValueLatticeElement {
     overdefined,
   };
 
-  ValueLatticeElementTy Tag;
+  ValueLatticeElementTy Tag : 6;
+  /// Number of times a constant range has been extended with widening enabled.
+  unsigned NumRangeExtensions : 8;
 
   /// The union either stores a pointer to a constant or a constant range,
   /// associated to the lattice element. We have to ensure that Range is
@@ -133,6 +135,7 @@ class ValueLatticeElement {
         new (&Range) ConstantRange(Other.Range);
       else
         Range = Other.Range;
+      NumRangeExtensions = Other.NumRangeExtensions;
       break;
     case constant:
     case notconstant:
@@ -287,7 +290,8 @@ class ValueLatticeElement {
   /// range or the object must be undef. The tag is set to
   /// constant_range_including_undef if either the existing value or the new
   /// range may include undef.
-  bool markConstantRange(ConstantRange NewR, bool MayIncludeUndef = false) {
+  bool markConstantRange(ConstantRange NewR, bool MayIncludeUndef = false,
+                         bool CheckWiden = false) {
     if (NewR.isFullSet())
       return markOverdefined();
 
@@ -304,6 +308,11 @@ class ValueLatticeElement {
       if (getConstantRange() == NewR)
         return Tag != OldTag;
 
+      // Simple form of widening. If a range is extended multiple times, go to
+      // overdefined.
+      if (CheckWiden && ++NumRangeExtensions == 1)
+        return markOverdefined();
+
       assert(NewR.contains(getConstantRange()) &&
              "Existing range must be a subset of NewR");
       Range = std::move(NewR);
@@ -314,6 +323,7 @@ class ValueLatticeElement {
     if (NewR.isEmptySet())
       return markOverdefined();
 
+    NumRangeExtensions = 0;
     Tag = NewTag;
     new (&Range) ConstantRange(std::move(NewR));
     return true;
@@ -321,7 +331,7 @@ class ValueLatticeElement {
 
   /// Updates this object to approximate both this object and RHS. Returns
   /// true if this object has been changed.
-  bool mergeIn(const ValueLatticeElement &RHS) {
+  bool mergeIn(const ValueLatticeElement &RHS, bool CheckWiden = false) {
     if (RHS.isUnknown() || isOverdefined())
       return false;
     if (RHS.isOverdefined()) {
@@ -337,7 +347,7 @@ class ValueLatticeElement {
         return markConstant(RHS.getConstant(), /*MayIncludeUndef=*/true);
       if (RHS.isConstantRange())
         return markConstantRange(RHS.getConstantRange(true),
-                                 /*MayIncludeUndef=*/true);
+                                 /*MayIncludeUndef=*/true, CheckWiden);
       return markOverdefined();
     }
 
@@ -380,7 +390,7 @@ class ValueLatticeElement {
     ConstantRange NewR = getConstantRange().unionWith(RHS.getConstantRange());
     return markConstantRange(
         std::move(NewR),
-        /*MayIncludeUndef=*/RHS.isConstantRangeIncludingUndef());
+        /*MayIncludeUndef=*/RHS.isConstantRangeIncludingUndef(), CheckWiden);
   }
 
   // Compares this symbolic value with Other using Pred and returns either
@@ -412,7 +422,9 @@ class ValueLatticeElement {
   }
 };
 
-raw_ostream &operator<<(raw_ostream &OS, const ValueLatticeElement &Val);
+static_assert(sizeof(ValueLatticeElement) <= 40,
+              "size of ValueLatticeElement changed unexpectedly");
 
+raw_ostream &operator<<(raw_ostream &OS, const ValueLatticeElement &Val);
 } // end namespace llvm
 #endif

diff  --git a/llvm/lib/Transforms/Scalar/SCCP.cpp b/llvm/lib/Transforms/Scalar/SCCP.cpp
index 9cb7f0695484..f5b3ebd1a002 100644
--- a/llvm/lib/Transforms/Scalar/SCCP.cpp
+++ b/llvm/lib/Transforms/Scalar/SCCP.cpp
@@ -401,15 +401,7 @@ class SCCPSolver : public InstVisitor<SCCPSolver> {
 
   bool mergeInValue(ValueLatticeElement &IV, Value *V,
                     ValueLatticeElement MergeWithV, bool Widen = true) {
-    // Do a simple form of widening, to avoid extending a range repeatedly in a
-    // loop. If IV is a constant range, it means we already set it once. If
-    // MergeWithV would extend IV, mark V as overdefined.
-    if (Widen && IV.isConstantRange() && MergeWithV.isConstantRange() &&
-        !IV.getConstantRange().contains(MergeWithV.getConstantRange())) {
-      markOverdefined(IV, V);
-      return true;
-    }
-    if (IV.mergeIn(MergeWithV)) {
+    if (IV.mergeIn(MergeWithV, Widen)) {
       pushToWorkList(IV, V);
       LLVM_DEBUG(dbgs() << "Merged " << MergeWithV << " into " << *V << " : "
                         << IV << "\n");