Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[InstCombine] Create a class to lazily track computed known bits #66611

Merged
merged 16 commits into from
Oct 17, 2023

Conversation

dc03
Copy link
Member

@dc03 dc03 commented Sep 18, 2023

This patch adds a new class "WithCache" which stores a pointer to
any type passable to computeKnownBits along with KnownBits
information which is computed on-demand when getKnownBits()
is called. This allows reusing the known bits information when it is
passed as an argument to multiple functions.

It also changes a few functions to accept WithCache(s) so that
known bits information computed in some callees can be propagated to
others from the top level visitAddSub caller.

This gives a speedup of 0.12%:
https://llvm-compile-time-tracker.com/compare.php?from=499d41cef2e7bbb65804f6a815b9fa8b27efce0f&to=fbea87f1f1e6d5552e2bc309f8e201a3af6d28ec&stat=instructions:u

@llvmbot
Copy link
Collaborator

llvmbot commented Sep 18, 2023

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-llvm-analysis

Changes

This patch adds a new class "CachedBitsValue" which stores a pointer to
a Value along with KnownBits information which is computed on-demand
when getKnownBits() is called. This allows reusing the known bits
information for a Value when it is passed to multiple functions where it
is required.

It also changes a few functions to accept a CachedBitsValue(s) so that
known bits information computed in some callees can be propagated to
others from the top level visitAddSub caller.

This gives a speedup of 0.12%:
https://llvm-compile-time-tracker.com/compare.php?from=b6fb6e9ea4e04ed379cf4901f8691cebac412142&to=03910f6a07b9a38bff620c12c0fdf9d7835c48ac&stat=instructions:u


Patch is 23.91 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/66611.diff

6 Files Affected:

  • (added) llvm/include/llvm/Analysis/CachedBitsValue.h (+188)
  • (modified) llvm/include/llvm/Analysis/ValueTracking.h (+21-8)
  • (modified) llvm/include/llvm/Transforms/InstCombine/InstCombiner.h (+4-3)
  • (modified) llvm/lib/Analysis/ValueTracking.cpp (+41-47)
  • (modified) llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp (+6-3)
  • (modified) llvm/lib/Transforms/InstCombine/InstCombineInternal.h (+4-2)
diff --git a/llvm/include/llvm/Analysis/CachedBitsValue.h b/llvm/include/llvm/Analysis/CachedBitsValue.h
new file mode 100644
index 000000000000000..0e13afc810da111
--- /dev/null
+++ b/llvm/include/llvm/Analysis/CachedBitsValue.h
@@ -0,0 +1,188 @@
+//===- llvm/Analysis/CachedBitsValue.h - Value with KnownBits - -*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// Store a pointer to an llvm::Value along with the KnownBits information for it
+// that is computed lazily (if required).
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_ANALYSIS_VALUEWITHINFOCACHE_H
+#define LLVM_ANALYSIS_VALUEWITHINFOCACHE_H
+
+#include "llvm/IR/Value.h"
+#include "llvm/Support/KnownBits.h"
+#include <type_traits>
+
+namespace llvm {
+
+class DataLayout;
+class AssumptionCache;
+class Instruction;
+class DominatorTree;
+struct SimplifyQuery;
+
+KnownBits computeKnownBits(const Value *V, const APInt &DemandedElts,
+                           unsigned Depth, const SimplifyQuery &Q);
+
+KnownBits computeKnownBits(const Value *V, unsigned Depth,
+                           const SimplifyQuery &Q);
+
+namespace detail {
+/// Represents a pointer to an llvm::Value with known bits information
+template <bool ConstPointer = true> class ImplCachedBitsValue {
+protected:
+  using ValuePointerType =
+      std::conditional_t<ConstPointer, const Value *, Value *>;
+  using ValueReferenceType =
+      std::conditional_t<ConstPointer, const Value &, Value &>;
+
+  template <typename T>
+  constexpr static bool ValuePointerConvertible =
+      std::is_convertible_v<T, ValuePointerType>;
+
+  ValuePointerType Pointer;
+  mutable KnownBits Known;
+  mutable bool HasKnownBits;
+
+  void calculateKnownBits(unsigned Depth, const SimplifyQuery &Q) const {
+    HasKnownBits = true;
+    Known = computeKnownBits(Pointer, Depth, Q);
+  }
+
+  void calculateKnownBits(const APInt &DemandedElts, unsigned Depth,
+                          const SimplifyQuery &Q) const {
+    HasKnownBits = true;
+    Known = computeKnownBits(Pointer, DemandedElts, Depth, Q);
+  }
+
+public:
+  ImplCachedBitsValue() = default;
+  ImplCachedBitsValue(ValuePointerType Pointer)
+      : Pointer(Pointer), Known(), HasKnownBits(false) {}
+  ImplCachedBitsValue(ValuePointerType Pointer, const KnownBits &Known)
+      : Pointer(Pointer), Known(Known), HasKnownBits(true) {}
+
+  template <typename T, std::enable_if_t<ValuePointerConvertible<T>, int> = 0>
+  ImplCachedBitsValue(const T &Value)
+      : Pointer(static_cast<ValuePointerType>(Value)), Known(),
+        HasKnownBits(false) {}
+
+  template <typename T, std::enable_if_t<ValuePointerConvertible<T>, int> = 0>
+  ImplCachedBitsValue(const T &Value, const KnownBits &Known)
+      : Pointer(static_cast<ValuePointerType>(Value)), Known(Known),
+        HasKnownBits(true) {}
+
+  ImplCachedBitsValue(ImplCachedBitsValue &&) = default;
+  ImplCachedBitsValue &operator=(ImplCachedBitsValue &&) = default;
+  ImplCachedBitsValue(const ImplCachedBitsValue &) = default;
+  ImplCachedBitsValue &operator=(const ImplCachedBitsValue &) = default;
+
+  ~ImplCachedBitsValue() = default;
+
+  [[nodiscard]] ValuePointerType getValue() { return Pointer; }
+  [[nodiscard]] ValuePointerType getValue() const { return Pointer; }
+
+  [[nodiscard]] const KnownBits &getKnownBits(unsigned Depth,
+                                              const SimplifyQuery &Q) const {
+    if (!hasKnownBits())
+      calculateKnownBits(Depth, Q);
+    return Known;
+  }
+
+  [[nodiscard]] KnownBits &getKnownBits(unsigned Depth,
+                                        const SimplifyQuery &Q) {
+    if (!hasKnownBits())
+      calculateKnownBits(Depth, Q);
+    return Known;
+  }
+
+  [[nodiscard]] const KnownBits &getKnownBits(const APInt &DemandedElts,
+                                              unsigned Depth,
+                                              const SimplifyQuery &Q) const {
+    if (!hasKnownBits())
+      calculateKnownBits(DemandedElts, Depth, Q);
+    return Known;
+  }
+
+  [[nodiscard]] KnownBits &getKnownBits(const APInt &DemandedElts,
+                                        unsigned Depth,
+                                        const SimplifyQuery &Q) {
+    if (!hasKnownBits())
+      calculateKnownBits(DemandedElts, Depth, Q);
+    return Known;
+  }
+
+  [[nodiscard]] bool hasKnownBits() const { return HasKnownBits; }
+
+  operator ValuePointerType() { return Pointer; }
+  ValuePointerType operator->() { return Pointer; }
+  ValueReferenceType operator*() { return *Pointer; }
+
+  operator ValuePointerType() const { return Pointer; }
+  ValuePointerType operator->() const { return Pointer; }
+  ValueReferenceType operator*() const { return *Pointer; }
+};
+} // namespace detail
+
+class CachedBitsConstValue : public detail::ImplCachedBitsValue<true> {
+public:
+  CachedBitsConstValue() = default;
+  CachedBitsConstValue(ValuePointerType Pointer)
+      : ImplCachedBitsValue(Pointer) {}
+  CachedBitsConstValue(Value *Pointer) : ImplCachedBitsValue(Pointer) {}
+  CachedBitsConstValue(ValuePointerType Pointer, const KnownBits &Known)
+      : ImplCachedBitsValue(Pointer, Known) {}
+
+  template <typename T, std::enable_if_t<ValuePointerConvertible<T>, int> = 0>
+  CachedBitsConstValue(const T &Value) : ImplCachedBitsValue(Value) {}
+
+  template <typename T, std::enable_if_t<ValuePointerConvertible<T>, int> = 0>
+  CachedBitsConstValue(const T &Value, const KnownBits &Known)
+      : ImplCachedBitsValue(Value, Known) {}
+
+  CachedBitsConstValue(CachedBitsConstValue &&) = default;
+  CachedBitsConstValue &operator=(CachedBitsConstValue &&) = default;
+  CachedBitsConstValue(const CachedBitsConstValue &) = default;
+  CachedBitsConstValue &operator=(const CachedBitsConstValue &) = default;
+
+  ~CachedBitsConstValue() = default;
+};
+
+class CachedBitsNonConstValue : public detail::ImplCachedBitsValue<false> {
+public:
+  CachedBitsNonConstValue() = default;
+  CachedBitsNonConstValue(ValuePointerType Pointer)
+      : ImplCachedBitsValue(Pointer) {}
+  CachedBitsNonConstValue(ValuePointerType Pointer, const KnownBits &Known)
+      : ImplCachedBitsValue(Pointer, Known) {}
+
+  template <typename T, std::enable_if_t<ValuePointerConvertible<T>, int> = 0>
+  CachedBitsNonConstValue(const T &Value) : ImplCachedBitsValue(Value) {}
+
+  template <typename T, std::enable_if_t<ValuePointerConvertible<T>, int> = 0>
+  CachedBitsNonConstValue(const T &Value, const KnownBits &Known)
+      : ImplCachedBitsValue(Value, Known) {}
+
+  CachedBitsNonConstValue(CachedBitsNonConstValue &&) = default;
+  CachedBitsNonConstValue &operator=(CachedBitsNonConstValue &&) = default;
+  CachedBitsNonConstValue(const CachedBitsNonConstValue &) = default;
+  CachedBitsNonConstValue &operator=(const CachedBitsNonConstValue &) = default;
+
+  ~CachedBitsNonConstValue() = default;
+
+  [[nodiscard]] CachedBitsConstValue toConst() const {
+    if (hasKnownBits())
+      return CachedBitsConstValue(getValue(), Known);
+    else
+      return CachedBitsConstValue(getValue());
+  }
+};
+
+} // namespace llvm
+
+#endif
diff --git a/llvm/include/llvm/Analysis/ValueTracking.h b/llvm/include/llvm/Analysis/ValueTracking.h
index 695f2fecae885b7..40827c5119f62bf 100644
--- a/llvm/include/llvm/Analysis/ValueTracking.h
+++ b/llvm/include/llvm/Analysis/ValueTracking.h
@@ -16,6 +16,7 @@
 
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/SmallSet.h"
+#include "llvm/Analysis/CachedBitsValue.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/FMF.h"
@@ -90,6 +91,18 @@ KnownBits computeKnownBits(const Value *V, const APInt &DemandedElts,
                            const DominatorTree *DT = nullptr,
                            bool UseInstrInfo = true);
 
+void computeKnownBits(const Value *V, const APInt &DemandedElts,
+                      KnownBits &Known, unsigned Depth, const SimplifyQuery &Q);
+
+void computeKnownBits(const Value *V, KnownBits &Known, unsigned Depth,
+                      const SimplifyQuery &Q);
+
+KnownBits computeKnownBits(const Value *V, const APInt &DemandedElts,
+                           unsigned Depth, const SimplifyQuery &Q);
+
+KnownBits computeKnownBits(const Value *V, unsigned Depth,
+                           const SimplifyQuery &Q);
+
 /// Compute known bits from the range metadata.
 /// \p KnownZero the set of bits that are known to be zero
 /// \p KnownOne the set of bits that are known to be one
@@ -107,7 +120,8 @@ KnownBits analyzeKnownBitsFromAndXorOr(
     bool UseInstrInfo = true);
 
 /// Return true if LHS and RHS have no common bits set.
-bool haveNoCommonBitsSet(const Value *LHS, const Value *RHS,
+bool haveNoCommonBitsSet(const CachedBitsConstValue &LHSCache,
+                         const CachedBitsConstValue &RHSCache,
                          const DataLayout &DL, AssumptionCache *AC = nullptr,
                          const Instruction *CxtI = nullptr,
                          const DominatorTree *DT = nullptr,
@@ -858,13 +872,12 @@ OverflowResult computeOverflowForSignedMul(const Value *LHS, const Value *RHS,
                                            const Instruction *CxtI,
                                            const DominatorTree *DT,
                                            bool UseInstrInfo = true);
-OverflowResult computeOverflowForUnsignedAdd(const Value *LHS, const Value *RHS,
-                                             const DataLayout &DL,
-                                             AssumptionCache *AC,
-                                             const Instruction *CxtI,
-                                             const DominatorTree *DT,
-                                             bool UseInstrInfo = true);
-OverflowResult computeOverflowForSignedAdd(const Value *LHS, const Value *RHS,
+OverflowResult computeOverflowForUnsignedAdd(
+    const CachedBitsConstValue &LHS, const CachedBitsConstValue &RHS,
+    const DataLayout &DL, AssumptionCache *AC, const Instruction *CxtI,
+    const DominatorTree *DT, bool UseInstrInfo = true);
+OverflowResult computeOverflowForSignedAdd(const CachedBitsConstValue &LHS,
+                                           const CachedBitsConstValue &RHS,
                                            const DataLayout &DL,
                                            AssumptionCache *AC = nullptr,
                                            const Instruction *CxtI = nullptr,
diff --git a/llvm/include/llvm/Transforms/InstCombine/InstCombiner.h b/llvm/include/llvm/Transforms/InstCombine/InstCombiner.h
index b16a60c423ab103..e5f3d07fe2c1412 100644
--- a/llvm/include/llvm/Transforms/InstCombine/InstCombiner.h
+++ b/llvm/include/llvm/Transforms/InstCombine/InstCombiner.h
@@ -505,13 +505,14 @@ class LLVM_LIBRARY_VISIBILITY InstCombiner {
     return llvm::computeOverflowForSignedMul(LHS, RHS, DL, &AC, CxtI, &DT);
   }
 
-  OverflowResult computeOverflowForUnsignedAdd(const Value *LHS,
-                                               const Value *RHS,
+  OverflowResult computeOverflowForUnsignedAdd(const CachedBitsConstValue &LHS,
+                                               const CachedBitsConstValue &RHS,
                                                const Instruction *CxtI) const {
     return llvm::computeOverflowForUnsignedAdd(LHS, RHS, DL, &AC, CxtI, &DT);
   }
 
-  OverflowResult computeOverflowForSignedAdd(const Value *LHS, const Value *RHS,
+  OverflowResult computeOverflowForSignedAdd(const CachedBitsConstValue &LHS,
+                                             const CachedBitsConstValue &RHS,
                                              const Instruction *CxtI) const {
     return llvm::computeOverflowForSignedAdd(LHS, RHS, DL, &AC, CxtI, &DT);
   }
diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp
index c4153b824c37e0a..90233788331f41b 100644
--- a/llvm/lib/Analysis/ValueTracking.cpp
+++ b/llvm/lib/Analysis/ValueTracking.cpp
@@ -25,6 +25,7 @@
 #include "llvm/Analysis/AliasAnalysis.h"
 #include "llvm/Analysis/AssumeBundleQueries.h"
 #include "llvm/Analysis/AssumptionCache.h"
+#include "llvm/Analysis/CachedBitsValue.h"
 #include "llvm/Analysis/ConstantFolding.h"
 #include "llvm/Analysis/GuardUtils.h"
 #include "llvm/Analysis/InstructionSimplify.h"
@@ -144,12 +145,8 @@ static bool getShuffleDemandedElts(const ShuffleVectorInst *Shuf,
                                       DemandedElts, DemandedLHS, DemandedRHS);
 }
 
-static void computeKnownBits(const Value *V, const APInt &DemandedElts,
-                             KnownBits &Known, unsigned Depth,
-                             const SimplifyQuery &Q);
-
-static void computeKnownBits(const Value *V, KnownBits &Known, unsigned Depth,
-                             const SimplifyQuery &Q) {
+void llvm::computeKnownBits(const Value *V, KnownBits &Known, unsigned Depth,
+                            const SimplifyQuery &Q) {
   // Since the number of lanes in a scalable vector is unknown at compile time,
   // we track one bit which is implicitly broadcast to all lanes.  This means
   // that all lanes in a scalable vector are considered demanded.
@@ -178,12 +175,6 @@ void llvm::computeKnownBits(const Value *V, const APInt &DemandedElts,
                                    safeCxtI(V, CxtI), UseInstrInfo));
 }
 
-static KnownBits computeKnownBits(const Value *V, const APInt &DemandedElts,
-                                  unsigned Depth, const SimplifyQuery &Q);
-
-static KnownBits computeKnownBits(const Value *V, unsigned Depth,
-                                  const SimplifyQuery &Q);
-
 KnownBits llvm::computeKnownBits(const Value *V, const DataLayout &DL,
                                  unsigned Depth, AssumptionCache *AC,
                                  const Instruction *CxtI,
@@ -202,10 +193,14 @@ KnownBits llvm::computeKnownBits(const Value *V, const APInt &DemandedElts,
                                           safeCxtI(V, CxtI), UseInstrInfo));
 }
 
-bool llvm::haveNoCommonBitsSet(const Value *LHS, const Value *RHS,
+bool llvm::haveNoCommonBitsSet(const CachedBitsConstValue &LHSCache,
+                               const CachedBitsConstValue &RHSCache,
                                const DataLayout &DL, AssumptionCache *AC,
                                const Instruction *CxtI, const DominatorTree *DT,
                                bool UseInstrInfo) {
+  const Value *LHS = LHSCache.getValue();
+  const Value *RHS = RHSCache.getValue();
+
   assert(LHS->getType() == RHS->getType() &&
          "LHS and RHS should have the same type");
   assert(LHS->getType()->isIntOrIntVectorTy() &&
@@ -253,12 +248,14 @@ bool llvm::haveNoCommonBitsSet(const Value *LHS, const Value *RHS,
         match(LHS, m_Not(m_c_Or(m_Specific(A), m_Specific(B)))))
       return true;
   }
-  IntegerType *IT = cast<IntegerType>(LHS->getType()->getScalarType());
-  KnownBits LHSKnown(IT->getBitWidth());
-  KnownBits RHSKnown(IT->getBitWidth());
-  computeKnownBits(LHS, LHSKnown, DL, 0, AC, CxtI, DT, UseInstrInfo);
-  computeKnownBits(RHS, RHSKnown, DL, 0, AC, CxtI, DT, UseInstrInfo);
-  return KnownBits::haveNoCommonBitsSet(LHSKnown, RHSKnown);
+
+  return KnownBits::haveNoCommonBitsSet(
+      LHSCache.getKnownBits(0, SimplifyQuery(DL, /*TLI*/ nullptr, DT, AC,
+                                             safeCxtI(LHSCache, CxtI),
+                                             UseInstrInfo)),
+      RHSCache.getKnownBits(0, SimplifyQuery(DL, /*TLI*/ nullptr, DT, AC,
+                                             safeCxtI(RHSCache, CxtI),
+                                             UseInstrInfo)));
 }
 
 bool llvm::isOnlyUsedInZeroEqualityComparison(const Instruction *I) {
@@ -1778,8 +1775,8 @@ static void computeKnownBitsFromOperator(const Operator *I,
 
 /// Determine which bits of V are known to be either zero or one and return
 /// them.
-KnownBits computeKnownBits(const Value *V, const APInt &DemandedElts,
-                           unsigned Depth, const SimplifyQuery &Q) {
+KnownBits llvm::computeKnownBits(const Value *V, const APInt &DemandedElts,
+                                 unsigned Depth, const SimplifyQuery &Q) {
   KnownBits Known(getBitWidth(V->getType(), Q.DL));
   computeKnownBits(V, DemandedElts, Known, Depth, Q);
   return Known;
@@ -1787,8 +1784,8 @@ KnownBits computeKnownBits(const Value *V, const APInt &DemandedElts,
 
 /// Determine which bits of V are known to be either zero or one and return
 /// them.
-KnownBits computeKnownBits(const Value *V, unsigned Depth,
-                           const SimplifyQuery &Q) {
+KnownBits llvm::computeKnownBits(const Value *V, unsigned Depth,
+                                 const SimplifyQuery &Q) {
   KnownBits Known(getBitWidth(V->getType(), Q.DL));
   computeKnownBits(V, Known, Depth, Q);
   return Known;
@@ -1809,9 +1806,9 @@ KnownBits computeKnownBits(const Value *V, unsigned Depth,
 /// where V is a vector, known zero, and known one values are the
 /// same width as the vector element, and the bit is set only if it is true
 /// for all of the demanded elements in the vector specified by DemandedElts.
-void computeKnownBits(const Value *V, const APInt &DemandedElts,
-                      KnownBits &Known, unsigned Depth,
-                      const SimplifyQuery &Q) {
+void llvm::computeKnownBits(const Value *V, const APInt &DemandedElts,
+                            KnownBits &Known, unsigned Depth,
+                            const SimplifyQuery &Q) {
   if (!DemandedElts) {
     // No demanded elts, better to assume we don't know anything.
     Known.resetAll();
@@ -6251,11 +6248,13 @@ static OverflowResult mapOverflowResult(ConstantRange::OverflowResult OR) {
 
 /// Combine constant ranges from computeConstantRange() and computeKnownBits().
 static ConstantRange computeConstantRangeIncludingKnownBits(
-    const Value *V, bool ForSigned, const DataLayout &DL, unsigned Depth,
-    AssumptionCache *AC, const Instruction *CxtI, const DominatorTree *DT,
-    bool UseInstrInfo = true) {
-  KnownBits Known = computeKnownBits(V, DL, Depth, AC, CxtI, DT, UseInstrInfo);
-  ConstantRange CR1 = ConstantRange::fromKnownBits(Known, ForSigned);
+    const CachedBitsConstValue &V, bool ForSigned, const DataLayout &DL,
+    unsigned Depth, AssumptionCache *AC, const Instruction *CxtI,
+    const DominatorTree *DT, bool UseInstrInfo = true) {
+  ConstantRange CR1 = ConstantRange::fromKnownBits(
+      V.getKnownBits(Depth, SimplifyQuery(DL, /*TLI*/ nullptr, DT, AC,
+                                          safeCxtI(V, CxtI), UseInstrInfo)),
+      ForSigned);
   ConstantRange CR2 = computeConstantRange(V, ForSigned, UseInstrInfo);
   ConstantRange::PreferredRangeType RangeType =
       ForSigned ? ConstantRange::Signed : ConstantRange::Unsigned;
@@ -6319,9 +6318,9 @@ llvm::computeOverflowForSignedMul(const Value *LHS, const Value *RHS,
 }
 
 OverflowResult llvm::computeOverflowForUnsignedAdd(
-    const Value *LHS, const Value *RHS, const DataLayout &DL,
-    AssumptionCache *AC, const Instruction *CxtI, const DominatorTree *DT,
-    bool UseInstrInfo) {
+    const CachedBitsConstValue &LHS, const CachedBitsConstValue &RHS,
+    const DataLayout &DL, AssumptionCache *AC, const Instruction *CxtI,
+    const DominatorTree *DT, bool UseInstrInfo) {
   ConstantRange LHSRange = computeConstantRangeIncludingKnownBits(
       LHS, /*ForSigned=*/false, DL, /*Depth=*/0, AC, CxtI, DT, UseInstrInfo);
   ConstantRange RHSRange = computeConstantRangeIncludingKnownBits(
@@ -6329,13 +6328,10 @@ OverflowResult llvm::computeOverflowForUnsignedAdd(
   return mapOverflowResult(LHSRange.unsignedAddMayOverflow(RHSRange));
 }
 
-static OverflowResult computeOverflowForSignedAdd(const Value *LHS,
-                                                  const Value *RHS,
-                                                  const AddOperator *Add,
-                                                  const DataLayout &DL,
-                                                  AssumptionCache *AC,
-                                                  const Instruction *CxtI,
-                 ...
[truncated]

llvm/include/llvm/Analysis/CachedBitsValue.h Outdated Show resolved Hide resolved
llvm/include/llvm/Analysis/CachedBitsValue.h Outdated Show resolved Hide resolved
llvm/include/llvm/Analysis/CachedBitsValue.h Outdated Show resolved Hide resolved
llvm/include/llvm/Analysis/CachedBitsValue.h Outdated Show resolved Hide resolved

operator ValuePointerType() const { return Pointer; }
ValuePointerType operator->() const { return Pointer; }
ValueReferenceType operator*() const { return *Pointer; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it necessary to have const / non-const overloads of these?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was to make CachedBitsNonConstValue work (it's not necessary if that class is removed).

llvm/include/llvm/Analysis/CachedBitsValue.h Outdated Show resolved Hide resolved
@dc03 dc03 requested a review from nikic September 24, 2023 03:45
ConstantRange CR1 = ConstantRange::fromKnownBits(
V.getKnownBits(SimplifyQuery(DL, /*TLI*/ nullptr, DT, AC,
safeCxtI(V, CxtI), UseInstrInfo)),
ForSigned);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might just add an API for getCR() in the same vein is getKnownBits For now it can just do the above logic, but we might want to cache that someday as well. Also seems to fit.

@dc03
Copy link
Member Author

dc03 commented Oct 16, 2023

Sorry, had quite a busy week(end) so I wasn't able to work on this. I have added WithCache, and I also left CachedBitsValue around so that the review context is not lost. I will delete it when the review is done.

@github-actions
Copy link

github-actions bot commented Oct 16, 2023

✅ With the latest revision this PR passed the C/C++ code formatter.

Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks quite nice now.

llvm/include/llvm/Analysis/WithCache.h Outdated Show resolved Hide resolved
llvm/include/llvm/Analysis/WithCache.h Show resolved Hide resolved
: Pointer(Pointer, true), Known(Known) {}

template <typename T, std::enable_if_t<PointerConvertible<T>, int> = 0>
WithCache(const T &Value) : Pointer(static_cast<PointerType>(Value), false) {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it not sufficient to accept Arg Value here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are artifacts from CachedBitsValue, they aren't even ever called 🤦

llvm/include/llvm/Analysis/WithCache.h Outdated Show resolved Hide resolved
KnownBits Known = ::computeKnownBits(V, /*Depth=*/0, SQ);
ConstantRange CR1 = ConstantRange::fromKnownBits(Known, ForSigned);
ConstantRange CR1 =
ConstantRange::fromKnownBits(V.getKnownBits(SQ), ForSigned);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would still personally be in favor of making getCR an API of its own. But not particularly important.

Copy link
Contributor

@nikic nikic Oct 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we add a getConstantRange API on this class, then what it would have to return is a cached version of computeConstantRange(), not ConstantRange::fromKnownBits(). That's something we could evaluate, but not as part of this PR. I don't think it would be worthwhile right now (because, while we do repeated computeConstantRange() calls, they use different signedness, so couldn't be reused).

Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some more nits. Can you please also drop the old CachedBitsValue.h header?

llvm/include/llvm/Analysis/WithCache.h Outdated Show resolved Hide resolved
llvm/include/llvm/Analysis/WithCache.h Outdated Show resolved Hide resolved
llvm/include/llvm/Analysis/WithCache.h Outdated Show resolved Hide resolved
Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants