[llvm] [VN] be more consistent about forwarding null inputs and ignoring SVE outputs (PR #139574)
Jameson Nash via llvm-commits
llvm-commits at lists.llvm.org
Thu Jun 12 13:42:58 PDT 2025
https://github.com/vtjnash updated https://github.com/llvm/llvm-project/pull/139574
>From 8fc1d65f482d208cd0d5247bd1071527b97c97f5 Mon Sep 17 00:00:00 2001
From: Jameson Nash <vtjnash at gmail.com>
Date: Mon, 12 May 2025 14:30:17 +0000
Subject: [PATCH 1/2] [VN] be more consistent about forwarding null inputs
Many callers of canCoerceMustAliasedValueToLoad were duplicating checks
that should have been handled by isFirstClassAggregate, and were
therefore inconsistent about whether they would handle the null case.
Several callers were also relying on canCoerceMustAliasedValueToLoad to
reject all ScalableVectors (since they immediately need to call
getFixedValue on the result), but they need to handle that explicitly.
---
llvm/lib/Transforms/Scalar/GVN.cpp | 2 +-
llvm/lib/Transforms/Utils/VNCoercion.cpp | 60 +++++++++++++++---------
llvm/test/Transforms/GVN/store-null.ll | 16 +++++++
3 files changed, 56 insertions(+), 22 deletions(-)
create mode 100644 llvm/test/Transforms/GVN/store-null.ll
diff --git a/llvm/lib/Transforms/Scalar/GVN.cpp b/llvm/lib/Transforms/Scalar/GVN.cpp
index a0eed31fde792..e0256131227a7 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
+}
>From c281bf470ce354c6e30884176d5e589b0759c6de Mon Sep 17 00:00:00 2001
From: Jameson Nash <vtjnash at gmail.com>
Date: Thu, 12 Jun 2025 20:39:53 +0000
Subject: [PATCH 2/2] review fixes
---
llvm/lib/Transforms/Utils/VNCoercion.cpp | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/llvm/lib/Transforms/Utils/VNCoercion.cpp b/llvm/lib/Transforms/Utils/VNCoercion.cpp
index eac0a1687a662..1ab086d2e24b9 100644
--- a/llvm/lib/Transforms/Utils/VNCoercion.cpp
+++ b/llvm/lib/Transforms/Utils/VNCoercion.cpp
@@ -9,10 +9,6 @@
namespace llvm {
namespace VNCoercion {
-static bool isFirstClassAggregate(Type *Ty) {
- return Ty->isStructTy() || Ty->isArrayTy();
-}
-
/// Return true if coerceAvailableValueToLoadType will succeed.
bool canCoerceMustAliasedValueToLoad(Value *StoredVal, Type *LoadTy,
Function *F) {
@@ -40,7 +36,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;
}
@@ -61,7 +58,7 @@ bool canCoerceMustAliasedValueToLoad(Value *StoredVal, Type *LoadTy,
return true;
}
- if (isFirstClassAggregate(LoadTy) || isFirstClassAggregate(StoredTy))
+ if (LoadTy->isAggregateType() || StoredTy->isAggregateType())
return false;
bool StoredNI = DL.isNonIntegralPointerType(StoredTy->getScalarType());
@@ -325,7 +322,7 @@ static Value *getStoreValueForLoadHelper(Value *SrcVal, unsigned Offset,
// (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 (auto *CI = dyn_cast<Constant>(SrcVal))
if (CI->isNullValue())
return Constant::getNullValue(LoadTy);
More information about the llvm-commits
mailing list