[VN] be more consistent about forwarding null inputs and ignoring SVE outputs

[vtjnash]

Many callers of canCoerceMustAliasedValueToLoad were duplicating checks that should have already been handled by isFirstClassAggregate, and were therefore inconsistent about whether they would handle the zeroinitializer case. Instead, try to ensure all users handle the zeroinitializer specifically and consistently, so that non-integral pointers aren't the only case that gets optimized. And also avoids relying on the constant folder to legalize the insertion of (ptrtoint (ptr addrspace(NI) zeroinitializer) to isize) (or similar such vectors or also now arrays of the same).

Several callers were also noticed to be relying on canCoerceMustAliasedValueToLoad to reject all ScalableVectors (since they immediately tried to call getFixedValue on the type afterwards), but they needed to bail first. This shouldn't be a functional change in a release build (since it checked for SVE immediately afterward), but may avoid an assert in debugging. It also might be a future opportunity for improvement (since in some cases, it may not actually need the size explicitly, but just to check that the sizes are compatible).

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: Jameson Nash (vtjnash)

Changes

Many callers of canCoerceMustAliasedValueToLoad were duplicating checks that should have already been handled by isFirstClassAggregate, and were therefore inconsistent about whether they would handle the zeroinitializer case. Instead, try to ensure all users handle the zeroinitializer specifically and consistently, so that non-integral pointers aren't the only case that gets optimized. And also avoids relying on the constant folder to legalize the insertion of (ptrtoint (ptr addrspace(NI) zeroinitializer) to isize) (or similar such vectors or also now arrays of the same).

Several callers were also noticed to be relying on canCoerceMustAliasedValueToLoad to reject all ScalableVectors (since they immediately tried to call getFixedValue on the type afterwards), but they needed to bail first. This shouldn't be a functional change in a release build (since it checked for SVE immediately afterward), but may avoid an assert in debugging. It also might be a future opportunity for improvement (since in some cases, it may not actually need the size explicitly, but just to check that the sizes are compatible).

---

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

3 Files Affected:

• (modified) llvm/lib/Transforms/Scalar/GVN.cpp (+1-1)
• (modified) llvm/lib/Transforms/Utils/VNCoercion.cpp (+39-21)
• (added) llvm/test/Transforms/GVN/store-null.ll (+16)

diff --git a/llvm/lib/Transforms/Scalar/GVN.cpp b/llvm/lib/Transforms/Scalar/GVN.cpp
index 77ca14f88a834..735bf2aa76909 100644
--- a/llvm/lib/Transforms/Scalar/GVN.cpp
+++ b/llvm/lib/Transforms/Scalar/GVN.cpp
@@ -1346,7 +1346,7 @@ GVNPass::AnalyzeLoadAvailability(LoadInst *Load, MemDepResult DepInfo,
 
   if (StoreInst *S = dyn_cast<StoreInst>(DepInst)) {
     // Reject loads and stores that are to the same address but are of
-    // different types if we have to. If the stored value is convertable to
+    // different types if we have to. If the stored value is convertible to
     // the loaded value, we can reuse it.
     if (!canCoerceMustAliasedValueToLoad(S->getValueOperand(), Load->getType(),
                                          S->getFunction()))
diff --git a/llvm/lib/Transforms/Utils/VNCoercion.cpp b/llvm/lib/Transforms/Utils/VNCoercion.cpp
index c1bce01239dcc..eac0a1687a662 100644
--- a/llvm/lib/Transforms/Utils/VNCoercion.cpp
+++ b/llvm/lib/Transforms/Utils/VNCoercion.cpp
@@ -9,8 +9,8 @@
 namespace llvm {
 namespace VNCoercion {
 
-static bool isFirstClassAggregateOrScalableType(Type *Ty) {
-  return Ty->isStructTy() || Ty->isArrayTy() || isa<ScalableVectorType>(Ty);
+static bool isFirstClassAggregate(Type *Ty) {
+  return Ty->isStructTy() || Ty->isArrayTy();
 }
 
 /// Return true if coerceAvailableValueToLoadType will succeed.
@@ -40,28 +40,34 @@ bool canCoerceMustAliasedValueToLoad(Value *StoredVal, Type *LoadTy,
     unsigned MinVScale = Attrs.getVScaleRangeMin();
     MinStoreSize =
         TypeSize::getFixed(MinStoreSize.getKnownMinValue() * MinVScale);
-  } else if (isFirstClassAggregateOrScalableType(LoadTy) ||
-             isFirstClassAggregateOrScalableType(StoredTy)) {
+  } else if (isa<ScalableVectorType>(StoredTy) || isa<ScalableVectorType>(LoadTy)) {
     return false;
   }
 
   // The store size must be byte-aligned to support future type casts.
-  if (llvm::alignTo(MinStoreSize, 8) != MinStoreSize)
+  if (!MinStoreSize.isKnownMultipleOf(8))
     return false;
 
   // The store has to be at least as big as the load.
   if (!TypeSize::isKnownGE(MinStoreSize, LoadSize))
     return false;
 
+  // As a special case, allow coercion of memset used to initialize
+  // an array w/null. Despite non-integral pointers not generally having a
+  // specific bit pattern, we do assume null is zero.
+  if (StoredVal) {
+    Constant *CI = dyn_cast<Constant>(StoredVal);
+    if (CI && CI->isNullValue())
+      return true;
+  }
+
+  if (isFirstClassAggregate(LoadTy) || isFirstClassAggregate(StoredTy))
+    return false;
+
   bool StoredNI = DL.isNonIntegralPointerType(StoredTy->getScalarType());
   bool LoadNI = DL.isNonIntegralPointerType(LoadTy->getScalarType());
   // Don't coerce non-integral pointers to integers or vice versa.
   if (StoredNI != LoadNI) {
-    // As a special case, allow coercion of memset used to initialize
-    // an array w/null.  Despite non-integral pointers not generally having a
-    // specific bit pattern, we do assume null is zero.
-    if (auto *CI = dyn_cast<Constant>(StoredVal))
-      return CI->isNullValue();
     return false;
   } else if (StoredNI && LoadNI &&
              StoredTy->getPointerAddressSpace() !=
@@ -197,11 +203,6 @@ static int analyzeLoadFromClobberingWrite(Type *LoadTy, Value *LoadPtr,
                                           Value *WritePtr,
                                           uint64_t WriteSizeInBits,
                                           const DataLayout &DL) {
-  // If the loaded/stored value is a first class array/struct, or scalable type,
-  // don't try to transform them. We need to be able to bitcast to integer.
-  if (isFirstClassAggregateOrScalableType(LoadTy))
-    return -1;
-
   int64_t StoreOffset = 0, LoadOffset = 0;
   Value *StoreBase =
       GetPointerBaseWithConstantOffset(WritePtr, StoreOffset, DL);
@@ -235,8 +236,8 @@ int analyzeLoadFromClobberingStore(Type *LoadTy, Value *LoadPtr,
                                    StoreInst *DepSI, const DataLayout &DL) {
   auto *StoredVal = DepSI->getValueOperand();
 
-  // Cannot handle reading from store of first-class aggregate or scalable type.
-  if (isFirstClassAggregateOrScalableType(StoredVal->getType()))
+  // Cannot handle reading from store of scalable type.
+  if (isa<ScalableVectorType>(StoredVal->getType()))
     return -1;
 
   if (!canCoerceMustAliasedValueToLoad(StoredVal, LoadTy, DepSI->getFunction()))
@@ -244,7 +245,7 @@ int analyzeLoadFromClobberingStore(Type *LoadTy, Value *LoadPtr,
 
   Value *StorePtr = DepSI->getPointerOperand();
   uint64_t StoreSize =
-      DL.getTypeSizeInBits(DepSI->getValueOperand()->getType()).getFixedValue();
+      DL.getTypeSizeInBits(StoredVal->getType()).getFixedValue();
   return analyzeLoadFromClobberingWrite(LoadTy, LoadPtr, StorePtr, StoreSize,
                                         DL);
 }
@@ -254,8 +255,7 @@ int analyzeLoadFromClobberingStore(Type *LoadTy, Value *LoadPtr,
 /// the other load can feed into the second load.
 int analyzeLoadFromClobberingLoad(Type *LoadTy, Value *LoadPtr, LoadInst *DepLI,
                                   const DataLayout &DL) {
-  // Cannot handle reading from store of first-class aggregate or scalable type.
-  if (isFirstClassAggregateOrScalableType(DepLI->getType()))
+  if (isa<ScalableVectorType>(DepLI->getType()))
     return -1;
 
   if (!canCoerceMustAliasedValueToLoad(DepLI, LoadTy, DepLI->getFunction()))
@@ -274,8 +274,12 @@ int analyzeLoadFromClobberingMemInst(Type *LoadTy, Value *LoadPtr,
     return -1;
   uint64_t MemSizeInBits = SizeCst->getZExtValue() * 8;
 
+  // Cannot handle reading scalable type with unknown size.
+  if (isa<ScalableVectorType>(LoadTy))
+    return -1;
+
   // If this is memset, we just need to see if the offset is valid in the size
-  // of the memset..
+  // of the memset, since the src is a byte.
   if (const auto *memset_inst = dyn_cast<MemSetInst>(MI)) {
     if (DL.isNonIntegralPointerType(LoadTy->getScalarType())) {
       auto *CI = dyn_cast<ConstantInt>(memset_inst->getValue());
@@ -317,6 +321,13 @@ static Value *getStoreValueForLoadHelper(Value *SrcVal, unsigned Offset,
                                          Type *LoadTy, IRBuilderBase &Builder,
                                          const DataLayout &DL) {
   LLVMContext &Ctx = SrcVal->getType()->getContext();
+  // If CI is a null value, the intermediate code formed later might be invalid
+  // (e.g. creating a ptrtoint on NI addrspace), since it is a special case in
+  // canCoerceMustAliasedValueToLoad, so instead form the NullValue for the load
+  // directly
+  if (auto *CI = dyn_cast<Constant>(getUnderlyingObject(SrcVal)))
+    if (CI->isNullValue())
+      return Constant::getNullValue(LoadTy);
 
   // If two pointers are in the same address space, they have the same size,
   // so we don't need to do any truncation, etc. This avoids introducing
@@ -418,6 +429,13 @@ Value *getMemInstValueForLoad(MemIntrinsic *SrcInst, unsigned Offset,
     // memset(P, 'x', 1234) -> splat('x'), even if x is a variable, and
     // independently of what the offset is.
     Value *Val = MSI->getValue();
+    if (auto *CI = dyn_cast<Constant>(Val)) {
+      // memset(P, '\0', 1234) -> just directly create the null value for *P,
+      // by-passing any later validity checks (coerceAvailableValueToLoadType
+      // might try to insert an invalid CreateIntToPtr)
+      if (CI->isNullValue())
+        return Constant::getNullValue(LoadTy);
+    }
     if (LoadSize != 1)
       Val =
           Builder.CreateZExtOrBitCast(Val, IntegerType::get(Ctx, LoadSize * 8));
diff --git a/llvm/test/Transforms/GVN/store-null.ll b/llvm/test/Transforms/GVN/store-null.ll
new file mode 100644
index 0000000000000..64a4ac9638892
--- /dev/null
+++ b/llvm/test/Transforms/GVN/store-null.ll
@@ -0,0 +1,16 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
+; RUN: opt -passes=gvn -S -o - < %s | FileCheck %s
+
+define i32 @code() {
+; CHECK-LABEL: define i32 @code() {
+; CHECK-NEXT:  [[ENTRY:.*:]]
+; CHECK-NEXT:    [[META:%.*]] = alloca [3 x i32], align 4
+; CHECK-NEXT:    store [3 x i32] zeroinitializer, ptr [[META]], align 8
+; CHECK-NEXT:    ret i32 0
+;
+entry:
+  %meta = alloca [ 3 x i32 ], align 4
+  store [ 3 x i32 ] zeroinitializer, ptr %meta, align 8
+  %iload = load i32, ptr %meta, align 4
+  ret i32 %iload
+}

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff HEAD~1 HEAD --extensions cpp -- llvm/lib/Transforms/Scalar/GVN.cpp llvm/lib/Transforms/Utils/VNCoercion.cpp

View the diff from clang-format here.

diff --git a/llvm/lib/Transforms/Utils/VNCoercion.cpp b/llvm/lib/Transforms/Utils/VNCoercion.cpp
index eac0a1687..cb02dd576 100644
--- a/llvm/lib/Transforms/Utils/VNCoercion.cpp
+++ b/llvm/lib/Transforms/Utils/VNCoercion.cpp
@@ -40,7 +40,8 @@ bool canCoerceMustAliasedValueToLoad(Value *StoredVal, Type *LoadTy,
     unsigned MinVScale = Attrs.getVScaleRangeMin();
     MinStoreSize =
         TypeSize::getFixed(MinStoreSize.getKnownMinValue() * MinVScale);
-  } else if (isa<ScalableVectorType>(StoredTy) || isa<ScalableVectorType>(LoadTy)) {
+  } else if (isa<ScalableVectorType>(StoredTy) ||
+             isa<ScalableVectorType>(LoadTy)) {
     return false;
   }