[Analysis] Add Scalable field in MemoryLocation.h #69716

harviniriawan · 2023-10-20T12:43:52Z

This is the first of a series of patch to improve Alias Analysis on
Scalable quantities.
Keep Scalable information from TypeSize which
will be used in Alias Analysis.

harviniriawan · 2023-10-20T12:44:56Z

The same as #65759. I've deleted the old PR.

llvmbot · 2023-10-20T12:45:30Z

@llvm/pr-subscribers-llvm-analysis

Author: Harvin Iriawan (harviniriawan)

Changes

This is the first of a series of patch to improve Alias Analysis on
Scalable quantities.
Keep Scalable information from TypeSize which
will be used in Alias Analysis.

Full diff: https://github.com/llvm/llvm-project/pull/69716.diff

8 Files Affected:

(modified) llvm/include/llvm/Analysis/MemoryLocation.h (+36-12)
(modified) llvm/lib/Analysis/BasicAliasAnalysis.cpp (+34-21)
(modified) llvm/lib/Analysis/MemoryDependenceAnalysis.cpp (+4-2)
(modified) llvm/lib/CodeGen/StackProtector.cpp (+2-2)
(modified) llvm/lib/Transforms/IPO/AttributorAttributes.cpp (+2-1)
(modified) llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp (+19-10)
(added) llvm/test/Analysis/AliasSet/memloc-vscale.ll (+52)
(added) llvm/test/Transforms/GVN/scalable-memloc.ll (+29)

diff --git a/llvm/include/llvm/Analysis/MemoryLocation.h b/llvm/include/llvm/Analysis/MemoryLocation.h
index 85ca84e68a13971..8e24ec5c455b65b 100644
--- a/llvm/include/llvm/Analysis/MemoryLocation.h
+++ b/llvm/include/llvm/Analysis/MemoryLocation.h
@@ -64,16 +64,19 @@ class Value;
 //
 // If asked to represent a pathologically large value, this will degrade to
 // std::nullopt.
+// Store Scalable information in bit 62 of Value. Scalable information is
+// required to do Alias Analysis on Scalable quantities
 class LocationSize {
   enum : uint64_t {
     BeforeOrAfterPointer = ~uint64_t(0),
-    AfterPointer = BeforeOrAfterPointer - 1,
-    MapEmpty = BeforeOrAfterPointer - 2,
-    MapTombstone = BeforeOrAfterPointer - 3,
+    ScalableBit = uint64_t(1) << 62,
+    AfterPointer = (BeforeOrAfterPointer - 1) & ~ScalableBit,
+    MapEmpty = (BeforeOrAfterPointer - 2) & ~ScalableBit,
+    MapTombstone = (BeforeOrAfterPointer - 3) & ~ScalableBit,
     ImpreciseBit = uint64_t(1) << 63,
 
     // The maximum value we can represent without falling back to 'unknown'.
-    MaxValue = (MapTombstone - 1) & ~ImpreciseBit,
+    MaxValue = (MapTombstone - 1) & ~(ImpreciseBit | ScalableBit),
   };
 
   uint64_t Value;
@@ -88,6 +91,7 @@ class LocationSize {
                 "AfterPointer is imprecise by definition.");
   static_assert(BeforeOrAfterPointer & ImpreciseBit,
                 "BeforeOrAfterPointer is imprecise by definition.");
+  static_assert(~(MaxValue & ScalableBit), "Max value don't have bit 62 set");
 
 public:
   // FIXME: Migrate all users to construct via either `precise` or `upperBound`,
@@ -98,12 +102,17 @@ class LocationSize {
   // this assumes the provided value is precise.
   constexpr LocationSize(uint64_t Raw)
       : Value(Raw > MaxValue ? AfterPointer : Raw) {}
-
-  static LocationSize precise(uint64_t Value) { return LocationSize(Value); }
+  constexpr LocationSize(uint64_t Raw, bool Scalable)
+      : Value(Raw > MaxValue ? AfterPointer
+                             : Raw | (Scalable ? ScalableBit : uint64_t(0))) {}
+
+  // Make construction of LocationSize that takes in uint64_t to set Scalable
+  // information as false
+  static LocationSize precise(uint64_t Value) {
+    return LocationSize(Value, false /*Scalable*/);
+  }
   static LocationSize precise(TypeSize Value) {
-    if (Value.isScalable())
-      return afterPointer();
-    return precise(Value.getFixedValue());
+    return LocationSize(Value.getKnownMinValue(), Value.isScalable());
   }
 
   static LocationSize upperBound(uint64_t Value) {
@@ -150,6 +159,8 @@ class LocationSize {
       return beforeOrAfterPointer();
     if (Value == AfterPointer || Other.Value == AfterPointer)
       return afterPointer();
+    if (isScalable() || Other.isScalable())
+      return afterPointer();
 
     return upperBound(std::max(getValue(), Other.getValue()));
   }
@@ -157,9 +168,20 @@ class LocationSize {
   bool hasValue() const {
     return Value != AfterPointer && Value != BeforeOrAfterPointer;
   }
-  uint64_t getValue() const {
+  bool isScalable() const { return (Value & ScalableBit); }
+
+  uint64_t getUIntValue() const {
     assert(hasValue() && "Getting value from an unknown LocationSize!");
-    return Value & ~ImpreciseBit;
+    assert((Value & ~(ImpreciseBit | ScalableBit)) < MaxValue &&
+           "Scalable bit of value should be masked");
+    return Value & ~(ImpreciseBit | ScalableBit);
+  }
+
+  TypeSize getValue() const {
+    assert(hasValue() && "Getting value from an unknown LocationSize!");
+    assert((Value & ~(ImpreciseBit | ScalableBit)) < MaxValue &&
+           "Scalable bit of value should be masked");
+    return {Value & ~(ImpreciseBit | ScalableBit), isScalable()};
   }
 
   // Returns whether or not this value is precise. Note that if a value is
@@ -169,7 +191,9 @@ class LocationSize {
   }
 
   // Convenience method to check if this LocationSize's value is 0.
-  bool isZero() const { return hasValue() && getValue() == 0; }
+  bool isZero() const {
+    return hasValue() && getValue().getKnownMinValue() == 0;
+  }
 
   /// Whether accesses before the base pointer are possible.
   bool mayBeBeforePointer() const { return Value == BeforeOrAfterPointer; }
diff --git a/llvm/lib/Analysis/BasicAliasAnalysis.cpp b/llvm/lib/Analysis/BasicAliasAnalysis.cpp
index ca65abeb591c561..a37515a572c4fc3 100644
--- a/llvm/lib/Analysis/BasicAliasAnalysis.cpp
+++ b/llvm/lib/Analysis/BasicAliasAnalysis.cpp
@@ -101,22 +101,23 @@ bool BasicAAResult::invalidate(Function &Fn, const PreservedAnalyses &PA,
 //===----------------------------------------------------------------------===//
 
 /// Returns the size of the object specified by V or UnknownSize if unknown.
-static uint64_t getObjectSize(const Value *V, const DataLayout &DL,
-                              const TargetLibraryInfo &TLI,
-                              bool NullIsValidLoc,
-                              bool RoundToAlign = false) {
+/// getObjectSize does not support scalable Value
+static LocationSize getObjectSize(const Value *V, const DataLayout &DL,
+                                  const TargetLibraryInfo &TLI,
+                                  bool NullIsValidLoc,
+                                  bool RoundToAlign = false) {
   uint64_t Size;
   ObjectSizeOpts Opts;
   Opts.RoundToAlign = RoundToAlign;
   Opts.NullIsUnknownSize = NullIsValidLoc;
   if (getObjectSize(V, Size, DL, &TLI, Opts))
-    return Size;
-  return MemoryLocation::UnknownSize;
+    return LocationSize(Size);
+  return LocationSize(MemoryLocation::UnknownSize);
 }
 
 /// Returns true if we can prove that the object specified by V is smaller than
 /// Size.
-static bool isObjectSmallerThan(const Value *V, uint64_t Size,
+static bool isObjectSmallerThan(const Value *V, LocationSize Size,
                                 const DataLayout &DL,
                                 const TargetLibraryInfo &TLI,
                                 bool NullIsValidLoc) {
@@ -151,19 +152,21 @@ static bool isObjectSmallerThan(const Value *V, uint64_t Size,
 
   // This function needs to use the aligned object size because we allow
   // reads a bit past the end given sufficient alignment.
-  uint64_t ObjectSize = getObjectSize(V, DL, TLI, NullIsValidLoc,
-                                      /*RoundToAlign*/ true);
+  LocationSize ObjectSize = getObjectSize(V, DL, TLI, NullIsValidLoc,
+                                          /*RoundToAlign*/ true);
 
-  return ObjectSize != MemoryLocation::UnknownSize && ObjectSize < Size;
+  // Bail on comparing V and Size if Size is scalable
+  return ObjectSize != MemoryLocation::UnknownSize && !Size.isScalable() &&
+         ObjectSize.getValue() < Size.getValue();
 }
 
 /// Return the minimal extent from \p V to the end of the underlying object,
 /// assuming the result is used in an aliasing query. E.g., we do use the query
 /// location size and the fact that null pointers cannot alias here.
-static uint64_t getMinimalExtentFrom(const Value &V,
-                                     const LocationSize &LocSize,
-                                     const DataLayout &DL,
-                                     bool NullIsValidLoc) {
+static LocationSize getMinimalExtentFrom(const Value &V,
+                                         const LocationSize &LocSize,
+                                         const DataLayout &DL,
+                                         bool NullIsValidLoc) {
   // If we have dereferenceability information we know a lower bound for the
   // extent as accesses for a lower offset would be valid. We need to exclude
   // the "or null" part if null is a valid pointer. We can ignore frees, as an
@@ -175,15 +178,17 @@ static uint64_t getMinimalExtentFrom(const Value &V,
   // If queried with a precise location size, we assume that location size to be
   // accessed, thus valid.
   if (LocSize.isPrecise())
-    DerefBytes = std::max(DerefBytes, LocSize.getValue());
-  return DerefBytes;
+    DerefBytes = std::max(DerefBytes, LocSize.getValue().getKnownMinValue());
+  // FIXME: Is this right?
+  return LocationSize(DerefBytes, LocSize.isScalable());
 }
 
 /// Returns true if we can prove that the object specified by V has size Size.
-static bool isObjectSize(const Value *V, uint64_t Size, const DataLayout &DL,
+static bool isObjectSize(const Value *V, TypeSize Size, const DataLayout &DL,
                          const TargetLibraryInfo &TLI, bool NullIsValidLoc) {
-  uint64_t ObjectSize = getObjectSize(V, DL, TLI, NullIsValidLoc);
-  return ObjectSize != MemoryLocation::UnknownSize && ObjectSize == Size;
+  LocationSize ObjectSize = getObjectSize(V, DL, TLI, NullIsValidLoc);
+  return ObjectSize != MemoryLocation::UnknownSize &&
+         ObjectSize.getValue() == Size;
 }
 
 //===----------------------------------------------------------------------===//
@@ -1056,15 +1061,19 @@ AliasResult BasicAAResult::aliasGEP(
 
   // If an inbounds GEP would have to start from an out of bounds address
   // for the two to alias, then we can assume noalias.
+  // TODO: Remove !isScalable() once BasicAA fully support scalable location
+  // size
   if (*DecompGEP1.InBounds && DecompGEP1.VarIndices.empty() &&
-      V2Size.hasValue() && DecompGEP1.Offset.sge(V2Size.getValue()) &&
+      V2Size.hasValue() && !V2Size.isScalable() &&
+      DecompGEP1.Offset.sge(V2Size.getValue()) &&
       isBaseOfObject(DecompGEP2.Base))
     return AliasResult::NoAlias;
 
   if (isa<GEPOperator>(V2)) {
     // Symmetric case to above.
     if (*DecompGEP2.InBounds && DecompGEP1.VarIndices.empty() &&
-        V1Size.hasValue() && DecompGEP1.Offset.sle(-V1Size.getValue()) &&
+        V1Size.hasValue() && !V1Size.isScalable() &&
+        DecompGEP1.Offset.sle(-V1Size.getValue()) &&
         isBaseOfObject(DecompGEP1.Base))
       return AliasResult::NoAlias;
   }
@@ -1088,6 +1097,10 @@ AliasResult BasicAAResult::aliasGEP(
     return BaseAlias;
   }
 
+  // Bail on analysing scalable LocationSize
+  if (V1Size.isScalable() || V2Size.isScalable())
+    return AliasResult::MayAlias;
+
   // If there is a constant difference between the pointers, but the difference
   // is less than the size of the associated memory object, then we know
   // that the objects are partially overlapping.  If the difference is
diff --git a/llvm/lib/Analysis/MemoryDependenceAnalysis.cpp b/llvm/lib/Analysis/MemoryDependenceAnalysis.cpp
index 49a31a52cf0e850..84ccf04517c89ba 100644
--- a/llvm/lib/Analysis/MemoryDependenceAnalysis.cpp
+++ b/llvm/lib/Analysis/MemoryDependenceAnalysis.cpp
@@ -373,7 +373,8 @@ static bool canSkipClobberingStore(const StoreInst *SI,
     return false;
   if (MemoryLocation::get(SI).Size != MemLoc.Size)
     return false;
-  if (std::min(MemLocAlign, SI->getAlign()).value() < MemLoc.Size.getValue())
+  if (std::min(MemLocAlign, SI->getAlign()).value() <
+      MemLoc.Size.getUIntValue())
     return false;
 
   auto *LI = dyn_cast<LoadInst>(SI->getValueOperand());
@@ -1103,7 +1104,8 @@ bool MemoryDependenceResults::getNonLocalPointerDepFromBB(
         // be conservative.
         ThrowOutEverything =
             CacheInfo->Size.isPrecise() != Loc.Size.isPrecise() ||
-            CacheInfo->Size.getValue() < Loc.Size.getValue();
+            TypeSize::isKnownLT(CacheInfo->Size.getValue(),
+                                Loc.Size.getValue());
       } else {
         // For our purposes, unknown size > all others.
         ThrowOutEverything = !Loc.Size.hasValue();
diff --git a/llvm/lib/CodeGen/StackProtector.cpp b/llvm/lib/CodeGen/StackProtector.cpp
index 387b653f8815367..b24a9dcb88f3283 100644
--- a/llvm/lib/CodeGen/StackProtector.cpp
+++ b/llvm/lib/CodeGen/StackProtector.cpp
@@ -176,10 +176,10 @@ static bool HasAddressTaken(const Instruction *AI, TypeSize AllocSize,
     const auto *I = cast<Instruction>(U);
     // If this instruction accesses memory make sure it doesn't access beyond
     // the bounds of the allocated object.
+    // TODO: TypeSize::getFixed should be modified to adapt to scalable vectors
     std::optional<MemoryLocation> MemLoc = MemoryLocation::getOrNone(I);
     if (MemLoc && MemLoc->Size.hasValue() &&
-        !TypeSize::isKnownGE(AllocSize,
-                             TypeSize::getFixed(MemLoc->Size.getValue())))
+        !TypeSize::isKnownGE(AllocSize, MemLoc->Size.getValue()))
       return true;
     switch (I->getOpcode()) {
     case Instruction::Store:
diff --git a/llvm/lib/Transforms/IPO/AttributorAttributes.cpp b/llvm/lib/Transforms/IPO/AttributorAttributes.cpp
index 3808eef7ab4e27a..94759beaadda091 100644
--- a/llvm/lib/Transforms/IPO/AttributorAttributes.cpp
+++ b/llvm/lib/Transforms/IPO/AttributorAttributes.cpp
@@ -2531,7 +2531,8 @@ static int64_t getKnownNonNullAndDerefBytesForUse(
   }
 
   std::optional<MemoryLocation> Loc = MemoryLocation::getOrNone(I);
-  if (!Loc || Loc->Ptr != UseV || !Loc->Size.isPrecise() || I->isVolatile())
+  if (!Loc || Loc->Ptr != UseV || !Loc->Size.isPrecise() ||
+      Loc->Size.isScalable() || I->isVolatile())
     return 0;
 
   int64_t Offset;
diff --git a/llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp b/llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
index 0586ff191d94ed1..197656a97f27057 100644
--- a/llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
+++ b/llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
@@ -205,16 +205,16 @@ static bool isShortenableAtTheBeginning(Instruction *I) {
   return isa<AnyMemSetInst>(I);
 }
 
-static uint64_t getPointerSize(const Value *V, const DataLayout &DL,
-                               const TargetLibraryInfo &TLI,
-                               const Function *F) {
+static LocationSize getPointerSize(const Value *V, const DataLayout &DL,
+                                   const TargetLibraryInfo &TLI,
+                                   const Function *F) {
   uint64_t Size;
   ObjectSizeOpts Opts;
   Opts.NullIsUnknownSize = NullPointerIsDefined(F);
 
   if (getObjectSize(V, Size, DL, &TLI, Opts))
-    return Size;
-  return MemoryLocation::UnknownSize;
+    return LocationSize(Size);
+  return LocationSize(MemoryLocation::UnknownSize);
 }
 
 namespace {
@@ -951,9 +951,10 @@ struct DSEState {
     // case the size/offset of the dead store does not matter.
     if (DeadUndObj == KillingUndObj && KillingLocSize.isPrecise() &&
         isIdentifiedObject(KillingUndObj)) {
-      uint64_t KillingUndObjSize = getPointerSize(KillingUndObj, DL, TLI, &F);
-      if (KillingUndObjSize != MemoryLocation::UnknownSize &&
-          KillingUndObjSize == KillingLocSize.getValue())
+      LocationSize KillingUndObjSize =
+          getPointerSize(KillingUndObj, DL, TLI, &F);
+      if (KillingUndObjSize.hasValue() &&
+          KillingUndObjSize.getValue() == KillingLocSize.getValue())
         return OW_Complete;
     }
 
@@ -976,9 +977,17 @@ struct DSEState {
       return isMaskedStoreOverwrite(KillingI, DeadI, BatchAA);
     }
 
-    const uint64_t KillingSize = KillingLocSize.getValue();
-    const uint64_t DeadSize = DeadLoc.Size.getValue();
+    const TypeSize KillingSize = KillingLocSize.getValue();
+    const TypeSize DeadSize = DeadLoc.Size.getValue();
+    // Bail on doing Size comparison which depends on AA for now
+    // TODO: Remove AnyScalable once Alias Analysis deal with scalable vectors
+    const bool AnyScalable =
+        DeadSize.isScalable() || KillingLocSize.isScalable();
 
+    // TODO: Remove AnyScalable constraint once alias analysis fully support
+    // scalable quantities
+    if (AnyScalable)
+      return OW_Unknown;
     // Query the alias information
     AliasResult AAR = BatchAA.alias(KillingLoc, DeadLoc);
 
diff --git a/llvm/test/Analysis/AliasSet/memloc-vscale.ll b/llvm/test/Analysis/AliasSet/memloc-vscale.ll
new file mode 100644
index 000000000000000..8a83645ddaf9a87
--- /dev/null
+++ b/llvm/test/Analysis/AliasSet/memloc-vscale.ll
@@ -0,0 +1,52 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -S < %s -passes=print-alias-sets 2>&1 | FileCheck %s
+
+; CHECK-LABEL: Alias sets for function 'sn'
+; CHECK: AliasSet[{{.*}}, 1] must alias, Mod Pointers: (ptr %p, unknown after)
+define void @sn(ptr %p) {;
+  store <vscale x 2 x i64> zeroinitializer, ptr %p, align 2
+  store i64 0, ptr %p, align 2
+  ret void
+}
+
+; CHECK-LABEL: Alias sets for function 'ns'
+; CHECK: AliasSet[{{.*}}, 1] must alias, Mod Pointers: (ptr %p, unknown after)
+define void @ns(ptr %p) {
+  store i64 0, ptr %p, align 2
+  store <vscale x 2 x i64> zeroinitializer, ptr %p, align 2
+  ret void
+}
+
+; CHECK-LABEL: Alias sets for function 'ss':
+; CHECK: AliasSet[{{.*}}, 1] must alias, Mod       Pointers: (ptr %p, LocationSize::precise(vscale x 16))
+define void @ss(ptr %p) {
+  store <vscale x 2 x i64> zeroinitializer, ptr %p, align 2
+  store <vscale x 2 x i64> zeroinitializer, ptr %p, align 2
+  ret void
+}
+
+; CHECK-LABEL: Alias sets for function 'ss2':
+; CHECK: AliasSet[{{.*}}, 1] must alias, Mod       Pointers: (ptr %p, unknown after)
+define void @ss2(ptr %p) {
+  store <vscale x 2 x i64> zeroinitializer, ptr %p, align 2
+  store <vscale x 2 x i64> zeroinitializer, ptr %p, align 2
+  store <vscale x 4 x i64> zeroinitializer, ptr %p, align 2
+  ret void
+}
+; CHECK-LABEL: Alias sets for function 'son':
+; CHECK: AliasSet[{{.*}}, 2] may alias, Mod       Pointers: (ptr %g, LocationSize::precise(vscale x 16)), (ptr %p, LocationSize::precise(8))
+define void @son(ptr %p) {
+  %g = getelementptr i8, ptr %p, i64 8
+  store <vscale x 2 x i64> zeroinitializer, ptr %g, align 2
+  store i64 0, ptr %p, align 2
+  ret void
+}
+
+; CHECK-LABEL: Alias sets for function 'sno':
+; CHECK: AliasSet[{{.*}}, 2] may alias, Mod       Pointers: (ptr %p, LocationSize::precise(vscale x 16)), (ptr %g, LocationSize::precise(8))
+define void @sno(ptr %p) {
+  %g = getelementptr i8, ptr %p, i64 8
+  store <vscale x 2 x i64> zeroinitializer, ptr %p, align 2
+  store i64 0, ptr %g, align 2
+  ret void
+}
diff --git a/llvm/test/Transforms/GVN/scalable-memloc.ll b/llvm/test/Transforms/GVN/scalable-memloc.ll
new file mode 100644
index 000000000000000..23b4c19f280ca5d
--- /dev/null
+++ b/llvm/test/Transforms/GVN/scalable-memloc.ll
@@ -0,0 +1,29 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -S < %s -passes=gvn | FileCheck %s
+
+define void @test(i1 %cmp19, ptr %p) {
+; CHECK-LABEL: @test(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    br i1 [[CMP19:%.*]], label [[WHILE_BODY_LR_PH:%.*]], label [[FOR_COND_PREHEADER:%.*]]
+; CHECK:       while.body.lr.ph:
+; CHECK-NEXT:    [[DOTPRE1:%.*]] = load <vscale x 2 x double>, ptr [[P:%.*]], align 16
+; CHECK-NEXT:    [[TMP0:%.*]] = extractelement <vscale x 2 x double> [[DOTPRE1]], i64 0
+; CHECK-NEXT:    ret void
+; CHECK:       for.cond.preheader:
+; CHECK-NEXT:    [[DOTPRE:%.*]] = load double, ptr [[P]], align 8
+; CHECK-NEXT:    [[ADD:%.*]] = fadd double [[DOTPRE]], 0.000000e+00
+; CHECK-NEXT:    ret void
+;
+entry:
+  br i1 %cmp19, label %while.body.lr.ph, label %for.cond.preheader
+
+while.body.lr.ph:                                 ; preds = %entry
+  %.pre1 = load <vscale x 2 x double>, ptr %p, align 16
+  %0 = extractelement <vscale x 2 x double> %.pre1, i64 0
+  ret void
+
+for.cond.preheader:                               ; preds = %entry
+  %.pre = load double, ptr %p, align 8
+  %add = fadd double %.pre, 0.000000e+00
+  ret void
+}

nikic · 2023-10-20T12:46:09Z

Why submit another PR for this? Which one should be reviewed?

harviniriawan · 2023-10-20T12:52:44Z

This one please, I've closed the other PR.

llvm/include/llvm/Analysis/MemoryLocation.h

nikic · 2023-10-20T14:17:33Z

llvm/include/llvm/Analysis/MemoryLocation.h

+    ScalableBit = uint64_t(1) << 62,
+    AfterPointer = (BeforeOrAfterPointer - 1) & ~ScalableBit,
+    MapEmpty = (BeforeOrAfterPointer - 2) & ~ScalableBit,
+    MapTombstone = (BeforeOrAfterPointer - 3) & ~ScalableBit,


Do we need to exclude the ScalableBit for these? Doesn't look relevant.

it seems that it's needed, else some tests like llvm/test/Analysis/BasicAA/libfuncs.ll fails

Hm, how does it fail?

I guess ScalableBit could matter for AfterPointer, but it shouldn't matter for MapEmpty or MapTombstone.

llvm/include/llvm/Analysis/MemoryLocation.h

llvm/lib/Analysis/BasicAliasAnalysis.cpp

nikic · 2023-10-20T14:31:17Z

llvm/lib/Analysis/BasicAliasAnalysis.cpp

-    DerefBytes = std::max(DerefBytes, LocSize.getValue());
-  return DerefBytes;
+    DerefBytes = std::max(DerefBytes, LocSize.getValue().getKnownMinValue());
+  // FIXME: Is this right?


I don't think it is. You might combine the result from getPointerDereferenceableBytes with the scalable from from LocSize. I think you should just drop the scalable flag from the result here (this is conservatively correct).

llvm/lib/Analysis/MemoryDependenceAnalysis.cpp

nikic · 2023-10-20T14:34:59Z

llvm/lib/CodeGen/StackProtector.cpp

@@ -176,10 +176,10 @@ static bool HasAddressTaken(const Instruction *AI, TypeSize AllocSize,
    const auto *I = cast<Instruction>(U);
    // If this instruction accesses memory make sure it doesn't access beyond
    // the bounds of the allocated object.
+    // TODO: TypeSize::getFixed should be modified to adapt to scalable vectors


Outdated TODO?

llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp

github-actions · 2023-10-23T11:02:41Z

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

llvm/lib/Analysis/BasicAliasAnalysis.cpp

llvm/lib/Analysis/MemoryDependenceAnalysis.cpp

llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp

nikic

LGTM apart from some nits.

nikic · 2023-10-24T10:47:49Z

llvm/include/llvm/Analysis/MemoryLocation.h

+  // Make construction of LocationSize that takes in uint64_t to set Scalable
+  // information as false


Suggested change

// Make construction of LocationSize that takes in uint64_t to set Scalable

// information as false

/// Create non-scalable LocationSize.

nikic · 2023-10-24T10:48:14Z

llvm/lib/Analysis/BasicAliasAnalysis.cpp

  return std::nullopt;
 }

 /// Returns true if we can prove that the object specified by V is smaller than
 /// Size.
-static bool isObjectSmallerThan(const Value *V, uint64_t Size,
+static bool isObjectSmallerThan(const Value *V, LocationSize Size,


I think if you make this accept TypeSize instead, and then...

nikic · 2023-10-24T10:48:52Z

llvm/lib/Analysis/BasicAliasAnalysis.cpp

-    DerefBytes = std::max(DerefBytes, LocSize.getValue());
-  return DerefBytes;
+    DerefBytes = std::max(DerefBytes, LocSize.getValue().getKnownMinValue());
+  return LocationSize(DerefBytes);


...make this return TypeSize as well, things will be slightly cleaner.

I think std::max is not happy when any of the comparison made is with scalable vectors, because it fails with
LLVM ERROR: Invalid size request on a scalable vector at std::max . Should this be marked as a TODO?

That sounds like you forget the getKnownMinValue() call?

so at the end, should I make it return TypeSize::getFixed(DerefBytes) instead of TypeSize::get(DerefBytes, LocSize.isScalable()) ?

Because at first I was changing even DerefBytes to be of TypeSize

Yes, what I had in mind is to just use TypeSize::getFixed(DerefBytes) instead of LocationSize(DerefBytes), nothing more. More precisely handling scalable vectors can be a followup.

This is the first of a series of patch to improve Alias Analysis on Scalable quantities. Keep Scalable information from TypeSize which will be used in Alias Analysis.

topperc · 2023-10-24T19:45:40Z

llvm/include/llvm/Analysis/MemoryLocation.h

 class LocationSize {
  enum : uint64_t {
    BeforeOrAfterPointer = ~uint64_t(0),
-    AfterPointer = BeforeOrAfterPointer - 1,
+    ScalableBit = uint64_t(1) << 62,
+    AfterPointer = (BeforeOrAfterPointer - 1) & ~ScalableBit,


Why does AfterPointer have the ScalableBit cleared but none of the other sentinel values do?

#69716 (comment)

some tests seem to fail if I don't exclude the ScalableBit from AfterPointer

harviniriawan requested a review from davemgreen October 20, 2023 12:43

harviniriawan requested a review from nikic as a code owner October 20, 2023 12:43

llvmbot added llvm:analysis Includes value tracking, cost tables and constant folding llvm:transforms labels Oct 20, 2023

nikic reviewed Oct 20, 2023

View reviewed changes

harviniriawan mentioned this pull request Oct 20, 2023

[BasicAA] Add Vscale GEP decomposition on variable index #69152

Open

harviniriawan force-pushed the memloc_scalable_field branch from ccb7181 to 7583614 Compare October 23, 2023 11:17

nikic reviewed Oct 23, 2023

View reviewed changes

harviniriawan force-pushed the memloc_scalable_field branch from 7583614 to 21d47e8 Compare October 23, 2023 14:37

nikic approved these changes Oct 24, 2023

View reviewed changes

harviniriawan force-pushed the memloc_scalable_field branch from 21d47e8 to 076bb92 Compare October 24, 2023 14:34

[Analysis] Add Scalable field in MemoryLocation.h

8a5c9ec

This is the first of a series of patch to improve Alias Analysis on Scalable quantities. Keep Scalable information from TypeSize which will be used in Alias Analysis.

harviniriawan force-pushed the memloc_scalable_field branch from 076bb92 to 8a5c9ec Compare October 24, 2023 14:50

harviniriawan merged commit 211dc4a into llvm:main Oct 24, 2023

harviniriawan deleted the memloc_scalable_field branch October 24, 2023 17:19

topperc reviewed Oct 24, 2023

View reviewed changes

		// Make construction of LocationSize that takes in uint64_t to set Scalable
		// information as false

	// Make construction of LocationSize that takes in uint64_t to set Scalable
	// information as false
	/// Create non-scalable LocationSize.

[Analysis] Add Scalable field in MemoryLocation.h #69716

[Analysis] Add Scalable field in MemoryLocation.h #69716

Uh oh!

Conversation

harviniriawan commented Oct 20, 2023

Uh oh!

harviniriawan commented Oct 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Oct 20, 2023

Uh oh!

nikic commented Oct 20, 2023

Uh oh!

harviniriawan commented Oct 20, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Oct 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nikic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

harviniriawan commented Oct 20, 2023 •

edited

Loading

github-actions bot commented Oct 23, 2023 •

edited

Loading