[IR] Fix GEP offset computations for vector GEPs #75448
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
InstCombine is determining incorrect byte offsets for GEPs into vectors of overaligned elements. Add a new testcase showing this behavior, serving as precommit for a fix.
This prepares a fix to GEP offset computations on vectors
of overaligned elements.
We have many places that analyze GEP offsets using GEP iterators
with the following pattern:
GTI = gep_type_begin(ElemTy, Indices),
GTE = gep_type_end(ElemTy, Indices);
for (; GTI != GTE; ++GTI) {
if (StructType *STy = GTI.getStructTypeOrNull()) {
// handle struct
[..]
} else {
// handle sequential (outmost index, array, vector):
auto Stride = DL.getTypeAllocSize(GTI.getIndexedType());
Offset += Index * Size;
}
}
This is incorrect for vectors of types whose bit size does not
equal its alloc size (e.g. overaligned types), as vectors
always bit-pack their elements.
This patch introduces new functions generic_gep_type_iterator::isVector()
and generic_gep_type_iterator::getSequentialElementStride(const DataLayout &)
to fix these patterns without having to teach all these places about
the specifics of vector bit layouts. With these helpers, the pattern
above can be fixed by replacing the stride computation:
auto Stride = GTI.getSequentialElementStride(DL);
llvmbot
added
clang
|
@llvm/pr-subscribers-backend-x86 @llvm/pr-subscribers-backend-aarch64 Author: Jannik Silvanus (jasilvanus) ChangesVectors are always bit-packed and don't respect the elements' alignment This PR fixes many places that rely on an incorrect pattern This changes behavior for GEPs into vectors with element types for which the
Existing tests are unaffected, but a miscompilation of a new precommitted test is fixed. Patch is 28.39 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/75448.diff 28 Files Affected:
diff --git a/clang/lib/CodeGen/CGExprScalar.cpp b/clang/lib/CodeGen/CGExprScalar.cpp
index 41ad2ddac30d2d..f3eabd1ce224b8 100644
--- a/clang/lib/CodeGen/CGExprScalar.cpp
+++ b/clang/lib/CodeGen/CGExprScalar.cpp
@@ -5293,7 +5293,7 @@ static GEPOffsetAndOverflow EmitGEPOffsetInBytes(Value *BasePtr, Value *GEPVal,
// Otherwise this is array-like indexing. The local offset is the index
// multiplied by the element size.
auto *ElementSize = llvm::ConstantInt::get(
- IntPtrTy, DL.getTypeAllocSize(GTI.getIndexedType()));
+ IntPtrTy, GTI.getSequentialElementStride(DL)));
auto *IndexS = Builder.CreateIntCast(Index, IntPtrTy, /*isSigned=*/true);
LocalOffset = eval(BO_Mul, ElementSize, IndexS);
}
diff --git a/llvm/include/llvm/Analysis/TargetTransformInfoImpl.h b/llvm/include/llvm/Analysis/TargetTransformInfoImpl.h
index 1d8f523e9792ba..140838ff7c7c2d 100644
--- a/llvm/include/llvm/Analysis/TargetTransformInfoImpl.h
+++ b/llvm/include/llvm/Analysis/TargetTransformInfoImpl.h
@@ -1041,7 +1041,7 @@ class TargetTransformInfoImplCRTPBase : public TargetTransformInfoImplBase {
if (TargetType->isScalableTy())
return TTI::TCC_Basic;
int64_t ElementSize =
- DL.getTypeAllocSize(GTI.getIndexedType()).getFixedValue();
+ GTI.getSequentialElementStride(DL).getFixedValue();
if (ConstIdx) {
BaseOffset +=
ConstIdx->getValue().sextOrTrunc(PtrSizeBits) * ElementSize;
diff --git a/llvm/include/llvm/IR/GetElementPtrTypeIterator.h b/llvm/include/llvm/IR/GetElementPtrTypeIterator.h
index f3272327c3f8b2..5b63ccb182a842 100644
--- a/llvm/include/llvm/IR/GetElementPtrTypeIterator.h
+++ b/llvm/include/llvm/IR/GetElementPtrTypeIterator.h
@@ -16,6 +16,7 @@
#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/PointerUnion.h"
+#include "llvm/IR/DataLayout.h"
#include "llvm/IR/DerivedTypes.h"
#include "llvm/IR/Operator.h"
#include "llvm/IR/User.h"
@@ -30,7 +31,39 @@ template <typename ItTy = User::const_op_iterator>
class generic_gep_type_iterator {
ItTy OpIt;
- PointerUnion<StructType *, Type *> CurTy;
+ // We use two different mechanisms to store the type a GEP index applies to.
+ // In some cases, we need to know the outer aggregate type the index is
+ // applied within, e.g. a struct. In such cases, we store the aggregate type
+ // in the iterator, and derive the element type on the fly.
+ //
+ // However, this is not always possible, because for the outermost index there
+ // is no containing type. In such cases, or if the containing type is not
+ // relevant, e.g. for arrays, the element type is stored as Type* in CurTy.
+ //
+ // If CurTy contains a Type* value, this does not imply anything about the
+ // type itself, because it is the element type and not the outer type.
+ // In particular, Type* can be a struct type.
+ //
+ // Consider this example:
+ //
+ // %my.struct = type { i32, [ 4 x float ] }
+ // [...]
+ // %gep = getelementptr %my.struct, ptr %ptr, i32 10, i32 1, 32 3
+ //
+ // Iterating over the indices of this GEP, CurTy will contain the following
+ // values:
+ // * i32 10: The outer index always operates on the GEP value type.
+ // CurTy contains a Type* pointing at `%my.struct`.
+ // * i32 1: This index is within a struct.
+ // CurTy contains a StructType* pointing at `%my.struct`.
+ // * i32 3: This index is within an array. We reuse the "flat" indexing
+ // for arrays which is also used in the top level GEP index.
+ // CurTy contains a Type* pointing at `float`.
+ //
+ // Vectors are handled separately because the layout of vectors is different
+ // for overaligned elements: Vectors are always bit-packed, whereas arrays
+ // respect ABI alignment of the elements.
+ PointerUnion<StructType *, VectorType *, Type *> CurTy;
generic_gep_type_iterator() = default;
@@ -69,6 +102,8 @@ class generic_gep_type_iterator {
Type *getIndexedType() const {
if (auto *T = dyn_cast_if_present<Type *>(CurTy))
return T;
+ if (auto *VT = dyn_cast_if_present<VectorType *>(CurTy))
+ return VT->getElementType();
return cast<StructType *>(CurTy)->getTypeAtIndex(getOperand());
}
@@ -79,7 +114,7 @@ class generic_gep_type_iterator {
if (auto *ATy = dyn_cast<ArrayType>(Ty))
CurTy = ATy->getElementType();
else if (auto *VTy = dyn_cast<VectorType>(Ty))
- CurTy = VTy->getElementType();
+ CurTy = VTy;
else
CurTy = dyn_cast<StructType>(Ty);
++OpIt;
@@ -108,7 +143,23 @@ class generic_gep_type_iterator {
// that.
bool isStruct() const { return isa<StructType *>(CurTy); }
- bool isSequential() const { return isa<Type *>(CurTy); }
+ bool isVector() const { return isa<VectorType *>(CurTy); }
+ bool isSequential() const { return !isStruct(); }
+
+ // For sequential GEP indices (all except those into structs), the index value
+ // can be translated into a byte offset by multiplying with an element stride.
+ // This function returns this stride, which both depends on the element type,
+ // and the containing aggregate type, as vectors always tightly bit-pack their
+ // elements.
+ TypeSize getSequentialElementStride(const DataLayout &DL) const {
+ assert(isSequential());
+ Type *ElemTy = getIndexedType();
+ TypeSize ElemSizeInBits = isVector() ? DL.getTypeSizeInBits(ElemTy)
+ : DL.getTypeAllocSizeInBits(ElemTy);
+ // Check for invalid GEPs that are not byte-addressable.
+ assert(ElemSizeInBits.isKnownMultipleOf(8));
+ return ElemSizeInBits.divideCoefficientBy(8);
+ }
StructType *getStructType() const { return cast<StructType *>(CurTy); }
diff --git a/llvm/lib/Analysis/BasicAliasAnalysis.cpp b/llvm/lib/Analysis/BasicAliasAnalysis.cpp
index 3de147368f2346..9eb063f0ee8674 100644
--- a/llvm/lib/Analysis/BasicAliasAnalysis.cpp
+++ b/llvm/lib/Analysis/BasicAliasAnalysis.cpp
@@ -639,7 +639,7 @@ BasicAAResult::DecomposeGEPExpression(const Value *V, const DataLayout &DL,
continue;
// Don't attempt to analyze GEPs if the scalable index is not zero.
- TypeSize AllocTypeSize = DL.getTypeAllocSize(GTI.getIndexedType());
+ TypeSize AllocTypeSize = GTI.getSequentialElementStride(DL);
if (AllocTypeSize.isScalable()) {
Decomposed.Base = V;
return Decomposed;
@@ -650,7 +650,7 @@ BasicAAResult::DecomposeGEPExpression(const Value *V, const DataLayout &DL,
continue;
}
- TypeSize AllocTypeSize = DL.getTypeAllocSize(GTI.getIndexedType());
+ TypeSize AllocTypeSize = GTI.getSequentialElementStride(DL);
if (AllocTypeSize.isScalable()) {
Decomposed.Base = V;
return Decomposed;
diff --git a/llvm/lib/Analysis/InlineCost.cpp b/llvm/lib/Analysis/InlineCost.cpp
index 7096e06d925ade..1fa7badaa4fa01 100644
--- a/llvm/lib/Analysis/InlineCost.cpp
+++ b/llvm/lib/Analysis/InlineCost.cpp
@@ -1429,7 +1429,7 @@ bool CallAnalyzer::accumulateGEPOffset(GEPOperator &GEP, APInt &Offset) {
continue;
}
- APInt TypeSize(IntPtrWidth, DL.getTypeAllocSize(GTI.getIndexedType()));
+ APInt TypeSize(IntPtrWidth, GTI.getSequentialElementStride(DL));
Offset += OpC->getValue().sextOrTrunc(IntPtrWidth) * TypeSize;
}
return true;
diff --git a/llvm/lib/Analysis/Local.cpp b/llvm/lib/Analysis/Local.cpp
index 30757abeb09802..f5e080d2c78e65 100644
--- a/llvm/lib/Analysis/Local.cpp
+++ b/llvm/lib/Analysis/Local.cpp
@@ -64,7 +64,7 @@ Value *llvm::emitGEPOffset(IRBuilderBase *Builder, const DataLayout &DL,
// Convert to correct type.
if (Op->getType() != IntIdxTy)
Op = Builder->CreateIntCast(Op, IntIdxTy, true, Op->getName() + ".c");
- TypeSize TSize = DL.getTypeAllocSize(GTI.getIndexedType());
+ TypeSize TSize = GTI.getSequentialElementStride(DL);
if (TSize != TypeSize::getFixed(1)) {
Value *Scale = Builder->CreateTypeSize(IntIdxTy->getScalarType(), TSize);
if (IntIdxTy->isVectorTy())
diff --git a/llvm/lib/Analysis/LoopAccessAnalysis.cpp b/llvm/lib/Analysis/LoopAccessAnalysis.cpp
index 0894560fd07898..ed22267ffe706b 100644
--- a/llvm/lib/Analysis/LoopAccessAnalysis.cpp
+++ b/llvm/lib/Analysis/LoopAccessAnalysis.cpp
@@ -2703,7 +2703,10 @@ static unsigned getGEPInductionOperand(const GetElementPtrInst *Gep) {
// If it's a type with the same allocation size as the result of the GEP we
// can peel off the zero index.
- if (DL.getTypeAllocSize(GEPTI.getIndexedType()) != GEPAllocSize)
+ TypeSize ElemSize = GEPTI.isStruct()
+ ? DL.getTypeAllocSize(GEPTI.getIndexedType())
+ : GEPTI.getSequentialElementStride(DL);
+ if (ElemSize != GEPAllocSize)
break;
--LastOperand;
}
diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp
index 5445746ab2a1bc..c0a3dd7926b577 100644
--- a/llvm/lib/Analysis/ValueTracking.cpp
+++ b/llvm/lib/Analysis/ValueTracking.cpp
@@ -1230,7 +1230,7 @@ static void computeKnownBitsFromOperator(const Operator *I,
unsigned IndexBitWidth = Index->getType()->getScalarSizeInBits();
KnownBits IndexBits(IndexBitWidth);
computeKnownBits(Index, IndexBits, Depth + 1, Q);
- TypeSize IndexTypeSize = Q.DL.getTypeAllocSize(IndexedTy);
+ TypeSize IndexTypeSize = GTI.getSequentialElementStride(Q.DL);
uint64_t TypeSizeInBytes = IndexTypeSize.getKnownMinValue();
KnownBits ScalingFactor(IndexBitWidth);
// Multiply by current sizeof type.
@@ -2158,7 +2158,7 @@ static bool isGEPKnownNonNull(const GEPOperator *GEP, unsigned Depth,
}
// If we have a zero-sized type, the index doesn't matter. Keep looping.
- if (Q.DL.getTypeAllocSize(GTI.getIndexedType()).isZero())
+ if (GTI.getSequentialElementStride(Q.DL).isZero())
continue;
// Fast path the constant operand case both for efficiency and so we don't
diff --git a/llvm/lib/CodeGen/CodeGenPrepare.cpp b/llvm/lib/CodeGen/CodeGenPrepare.cpp
index f9e791c7334838..57f3497f21f91c 100644
--- a/llvm/lib/CodeGen/CodeGenPrepare.cpp
+++ b/llvm/lib/CodeGen/CodeGenPrepare.cpp
@@ -4787,7 +4787,7 @@ bool AddressingModeMatcher::matchOperationAddr(User *AddrInst, unsigned Opcode,
cast<ConstantInt>(AddrInst->getOperand(i))->getZExtValue();
ConstantOffset += SL->getElementOffset(Idx);
} else {
- TypeSize TS = DL.getTypeAllocSize(GTI.getIndexedType());
+ TypeSize TS = GTI.getSequentialElementStride(DL);
if (TS.isNonZero()) {
// The optimisations below currently only work for fixed offsets.
if (TS.isScalable())
diff --git a/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp b/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
index 27a53e55f32fa3..97b47e74803633 100644
--- a/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
@@ -1545,7 +1545,7 @@ bool IRTranslator::translateGetElementPtr(const User &U,
Offset += DL->getStructLayout(StTy)->getElementOffset(Field);
continue;
} else {
- uint64_t ElementSize = DL->getTypeAllocSize(GTI.getIndexedType());
+ uint64_t ElementSize = GTI.getSequentialElementStride(*DL);
// If this is a scalar constant or a splat vector of constants,
// handle it quickly.
diff --git a/llvm/lib/CodeGen/SelectionDAG/FastISel.cpp b/llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
index a831295863399b..3f668c815ae1ae 100644
--- a/llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
@@ -560,15 +560,13 @@ bool FastISel::selectGetElementPtr(const User *I) {
}
}
} else {
- Type *Ty = GTI.getIndexedType();
-
// If this is a constant subscript, handle it quickly.
if (const auto *CI = dyn_cast<ConstantInt>(Idx)) {
if (CI->isZero())
continue;
// N = N + Offset
uint64_t IdxN = CI->getValue().sextOrTrunc(64).getSExtValue();
- TotalOffs += DL.getTypeAllocSize(Ty) * IdxN;
+ TotalOffs += GTI.getSequentialElementStride(DL) * IdxN;
if (TotalOffs >= MaxOffs) {
N = fastEmit_ri_(VT, ISD::ADD, N, TotalOffs, VT);
if (!N) // Unhandled operand. Halt "fast" selection and bail.
@@ -585,7 +583,7 @@ bool FastISel::selectGetElementPtr(const User *I) {
}
// N = N + Idx * ElementSize;
- uint64_t ElementSize = DL.getTypeAllocSize(Ty);
+ uint64_t ElementSize = GTI.getSequentialElementStride(DL);
Register IdxN = getRegForGEPIndex(Idx);
if (!IdxN) // Unhandled operand. Halt "fast" selection and bail.
return false;
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
index 12ed4a82ee91a5..6005bb92a7970d 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
@@ -4112,7 +4112,7 @@ void SelectionDAGBuilder::visitGetElementPtr(const User &I) {
unsigned IdxSize = DAG.getDataLayout().getIndexSizeInBits(AS);
MVT IdxTy = MVT::getIntegerVT(IdxSize);
TypeSize ElementSize =
- DAG.getDataLayout().getTypeAllocSize(GTI.getIndexedType());
+ GTI.getSequentialElementStride(DAG.getDataLayout());
// We intentionally mask away the high bits here; ElementSize may not
// fit in IdxTy.
APInt ElementMul(IdxSize, ElementSize.getKnownMinValue());
diff --git a/llvm/lib/ExecutionEngine/Interpreter/Execution.cpp b/llvm/lib/ExecutionEngine/Interpreter/Execution.cpp
index 770fc93490835d..ae978070ac9f90 100644
--- a/llvm/lib/ExecutionEngine/Interpreter/Execution.cpp
+++ b/llvm/lib/ExecutionEngine/Interpreter/Execution.cpp
@@ -1074,7 +1074,7 @@ GenericValue Interpreter::executeGEPOperation(Value *Ptr, gep_type_iterator I,
assert(BitWidth == 64 && "Invalid index type for getelementptr");
Idx = (int64_t)IdxGV.IntVal.getZExtValue();
}
- Total += getDataLayout().getTypeAllocSize(I.getIndexedType()) * Idx;
+ Total += I.getSequentialElementStride(getDataLayout()) * Idx;
}
}
diff --git a/llvm/lib/IR/DataLayout.cpp b/llvm/lib/IR/DataLayout.cpp
index e28f043cf9e0d0..a2f5714c706874 100644
--- a/llvm/lib/IR/DataLayout.cpp
+++ b/llvm/lib/IR/DataLayout.cpp
@@ -936,9 +936,8 @@ int64_t DataLayout::getIndexedOffsetInType(Type *ElemTy,
// Add in the offset, as calculated by the structure layout info...
Result += Layout->getElementOffset(FieldNo);
} else {
- // Get the array index and the size of each array element.
- if (int64_t arrayIdx = cast<ConstantInt>(Idx)->getSExtValue())
- Result += arrayIdx * getTypeAllocSize(GTI.getIndexedType());
+ if (int64_t ArrayIdx = cast<ConstantInt>(Idx)->getSExtValue())
+ Result += ArrayIdx * GTI.getSequentialElementStride(*this);
}
}
diff --git a/llvm/lib/IR/Operator.cpp b/llvm/lib/IR/Operator.cpp
index cd982c7da102af..16a89534b4b3ec 100644
--- a/llvm/lib/IR/Operator.cpp
+++ b/llvm/lib/IR/Operator.cpp
@@ -87,7 +87,7 @@ Align GEPOperator::getMaxPreservedAlignment(const DataLayout &DL) const {
/// If the index isn't known, we take 1 because it is the index that will
/// give the worse alignment of the offset.
const uint64_t ElemCount = OpC ? OpC->getZExtValue() : 1;
- Offset = DL.getTypeAllocSize(GTI.getIndexedType()) * ElemCount;
+ Offset = GTI.getSequentialElementStride(DL) * ElemCount;
}
Result = Align(MinAlign(Offset, Result.value()));
}
@@ -157,7 +157,7 @@ bool GEPOperator::accumulateConstantOffset(
continue;
}
if (!AccumulateOffset(ConstOffset->getValue(),
- DL.getTypeAllocSize(GTI.getIndexedType())))
+ GTI.getSequentialElementStride(DL)))
return false;
continue;
}
@@ -170,8 +170,7 @@ bool GEPOperator::accumulateConstantOffset(
if (!ExternalAnalysis(*V, AnalysisIndex))
return false;
UsedExternalAnalysis = true;
- if (!AccumulateOffset(AnalysisIndex,
- DL.getTypeAllocSize(GTI.getIndexedType())))
+ if (!AccumulateOffset(AnalysisIndex, GTI.getSequentialElementStride(DL)))
return false;
}
return true;
@@ -218,14 +217,13 @@ bool GEPOperator::collectOffset(
continue;
}
CollectConstantOffset(ConstOffset->getValue(),
- DL.getTypeAllocSize(GTI.getIndexedType()));
+ GTI.getSequentialElementStride(DL));
continue;
}
if (STy || ScalableType)
return false;
- APInt IndexedSize =
- APInt(BitWidth, DL.getTypeAllocSize(GTI.getIndexedType()));
+ APInt IndexedSize = APInt(BitWidth, GTI.getSequentialElementStride(DL));
// Insert an initial offset of 0 for V iff none exists already, then
// increment the offset by IndexedSize.
if (!IndexedSize.isZero()) {
diff --git a/llvm/lib/IR/Value.cpp b/llvm/lib/IR/Value.cpp
index b6e25c46b514d8..94b0ae7435c949 100644
--- a/llvm/lib/IR/Value.cpp
+++ b/llvm/lib/IR/Value.cpp
@@ -1015,7 +1015,7 @@ getOffsetFromIndex(const GEPOperator *GEP, unsigned Idx, const DataLayout &DL) {
// Otherwise, we have a sequential type like an array or fixed-length
// vector. Multiply the index by the ElementSize.
- TypeSize Size = DL.getTypeAllocSize(GTI.getIndexedType());
+ TypeSize Size = GTI.getSequentialElementStride(DL);
if (Size.isScalable())
return std::nullopt;
Offset += Size.getFixedValue() * OpC->getSExtValue();
diff --git a/llvm/lib/Target/AArch64/AArch64FastISel.cpp b/llvm/lib/Target/AArch64/AArch64FastISel.cpp
index 9b8162ce8dd4d0..fcb781e922084c 100644
--- a/llvm/lib/Target/AArch64/AArch64FastISel.cpp
+++ b/llvm/lib/Target/AArch64/AArch64FastISel.cpp
@@ -645,7 +645,7 @@ bool AArch64FastISel::computeAddress(const Value *Obj, Address &Addr, Type *Ty)
unsigned Idx = cast<ConstantInt>(Op)->getZExtValue();
TmpOffset += SL->getElementOffset(Idx);
} else {
- uint64_t S = DL.getTypeAllocSize(GTI.getIndexedType());
+ uint64_t S = GTI.getSequentialElementStride(DL);
while (true) {
if (const ConstantInt *CI = dyn_cast<ConstantInt>(Op)) {
// Constant-offset addressing.
@@ -4987,15 +4987,13 @@ bool AArch64FastISel::selectGetElementPtr(const Instruction *I) {
if (Field)
TotalOffs += DL.getStructLayout(StTy)->getElementOffset(Field);
} else {
- Type *Ty = GTI.getIndexedType();
-
// If this is a constant subscript, handle it quickly.
if (const auto *CI = dyn_cast<ConstantInt>(Idx)) {
if (CI->isZero())
continue;
// N = N + Offset
- TotalOffs +=
- DL.getTypeAllocSize(Ty) * cast<ConstantInt>(CI)->getSExtValue();
+ TotalOffs += GTI.getSequentialElementStride(DL) *
+ cast<ConstantInt>(CI)->getSExtValue();
continue;
}
if (TotalOffs) {
@@ -5006,7 +5004,7 @@ bool AArch64FastISel::selectGetElementPtr(const Instruction *I) {
}
// N = N + Idx * ElementSize;
- uint64_t ElementSize = DL.getTypeAllocSize(Ty);
+ uint64_t ElementSize = GTI.getSequentialElementStride(DL);
unsigned IdxN = getRegForGEPIndex(Idx);
if (!IdxN)
return false;
diff --git a/llvm/lib/Target/ARM/ARMFastISel.cpp b/llvm/lib/Target/ARM/ARMFastISel.cpp
index 1d6aaeb7433b0f..cb3a709f7003bd 100644
--- a/llvm/lib/Target/ARM/ARMFastISel.cpp
+++ b/llvm/lib/Target/ARM/ARMFastISel.cpp
@@ -747,7 +747,7 @@ bool ARMFastISel::ARMComputeAddress(const Value *Obj, Address &Addr) {
unsigned Idx = cast<ConstantInt>(Op)->getZExtValue();
TmpOffset += SL->getElementOffset(Idx);
} else {
- uint64_t S = DL.getTypeAllocSize(GTI.getIndexedType());
+ uint64_t S = GTI.getSequentialElementStride(DL);
while (true) {
if (const ConstantInt *CI = dyn_cast<ConstantInt>(Op)) {
// Constant-offset addressing.
diff --git a/llvm/lib/Target/Mips/MipsFastISel.cpp b/llvm/lib/Target/Mips/MipsFastISel.cpp
index 7fcf375aa10b69..192ed1cec79a84 100644
--- a/llvm/lib/Target/Mips/MipsFastISel.cpp
+++ b/llvm/lib/Target/Mips/MipsFastISel.cpp
@@ -492,7 +...
[truncated]
|
|
@llvm/pr-subscribers-backend-arm Author: Jannik Silvanus (jasilvanus) ChangesVectors are always bit-packed and don't respect the elements' alignment This PR fixes many places that rely on an incorrect pattern This changes behavior for GEPs into vectors with element types for which the
Existing tests are unaffected, but a miscompilation of a new precommitted test is fixed. Patch is 28.39 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/75448.diff 28 Files Affected:
diff --git a/clang/lib/CodeGen/CGExprScalar.cpp b/clang/lib/CodeGen/CGExprScalar.cpp
index 41ad2ddac30d2d..f3eabd1ce224b8 100644
--- a/clang/lib/CodeGen/CGExprScalar.cpp
+++ b/clang/lib/CodeGen/CGExprScalar.cpp
@@ -5293,7 +5293,7 @@ static GEPOffsetAndOverflow EmitGEPOffsetInBytes(Value *BasePtr, Value *GEPVal,
// Otherwise this is array-like indexing. The local offset is the index
// multiplied by the element size.
auto *ElementSize = llvm::ConstantInt::get(
- IntPtrTy, DL.getTypeAllocSize(GTI.getIndexedType()));
+ IntPtrTy, GTI.getSequentialElementStride(DL)));
auto *IndexS = Builder.CreateIntCast(Index, IntPtrTy, /*isSigned=*/true);
LocalOffset = eval(BO_Mul, ElementSize, IndexS);
}
diff --git a/llvm/include/llvm/Analysis/TargetTransformInfoImpl.h b/llvm/include/llvm/Analysis/TargetTransformInfoImpl.h
index 1d8f523e9792ba..140838ff7c7c2d 100644
--- a/llvm/include/llvm/Analysis/TargetTransformInfoImpl.h
+++ b/llvm/include/llvm/Analysis/TargetTransformInfoImpl.h
@@ -1041,7 +1041,7 @@ class TargetTransformInfoImplCRTPBase : public TargetTransformInfoImplBase {
if (TargetType->isScalableTy())
return TTI::TCC_Basic;
int64_t ElementSize =
- DL.getTypeAllocSize(GTI.getIndexedType()).getFixedValue();
+ GTI.getSequentialElementStride(DL).getFixedValue();
if (ConstIdx) {
BaseOffset +=
ConstIdx->getValue().sextOrTrunc(PtrSizeBits) * ElementSize;
diff --git a/llvm/include/llvm/IR/GetElementPtrTypeIterator.h b/llvm/include/llvm/IR/GetElementPtrTypeIterator.h
index f3272327c3f8b2..5b63ccb182a842 100644
--- a/llvm/include/llvm/IR/GetElementPtrTypeIterator.h
+++ b/llvm/include/llvm/IR/GetElementPtrTypeIterator.h
@@ -16,6 +16,7 @@
#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/PointerUnion.h"
+#include "llvm/IR/DataLayout.h"
#include "llvm/IR/DerivedTypes.h"
#include "llvm/IR/Operator.h"
#include "llvm/IR/User.h"
@@ -30,7 +31,39 @@ template <typename ItTy = User::const_op_iterator>
class generic_gep_type_iterator {
ItTy OpIt;
- PointerUnion<StructType *, Type *> CurTy;
+ // We use two different mechanisms to store the type a GEP index applies to.
+ // In some cases, we need to know the outer aggregate type the index is
+ // applied within, e.g. a struct. In such cases, we store the aggregate type
+ // in the iterator, and derive the element type on the fly.
+ //
+ // However, this is not always possible, because for the outermost index there
+ // is no containing type. In such cases, or if the containing type is not
+ // relevant, e.g. for arrays, the element type is stored as Type* in CurTy.
+ //
+ // If CurTy contains a Type* value, this does not imply anything about the
+ // type itself, because it is the element type and not the outer type.
+ // In particular, Type* can be a struct type.
+ //
+ // Consider this example:
+ //
+ // %my.struct = type { i32, [ 4 x float ] }
+ // [...]
+ // %gep = getelementptr %my.struct, ptr %ptr, i32 10, i32 1, 32 3
+ //
+ // Iterating over the indices of this GEP, CurTy will contain the following
+ // values:
+ // * i32 10: The outer index always operates on the GEP value type.
+ // CurTy contains a Type* pointing at `%my.struct`.
+ // * i32 1: This index is within a struct.
+ // CurTy contains a StructType* pointing at `%my.struct`.
+ // * i32 3: This index is within an array. We reuse the "flat" indexing
+ // for arrays which is also used in the top level GEP index.
+ // CurTy contains a Type* pointing at `float`.
+ //
+ // Vectors are handled separately because the layout of vectors is different
+ // for overaligned elements: Vectors are always bit-packed, whereas arrays
+ // respect ABI alignment of the elements.
+ PointerUnion<StructType *, VectorType *, Type *> CurTy;
generic_gep_type_iterator() = default;
@@ -69,6 +102,8 @@ class generic_gep_type_iterator {
Type *getIndexedType() const {
if (auto *T = dyn_cast_if_present<Type *>(CurTy))
return T;
+ if (auto *VT = dyn_cast_if_present<VectorType *>(CurTy))
+ return VT->getElementType();
return cast<StructType *>(CurTy)->getTypeAtIndex(getOperand());
}
@@ -79,7 +114,7 @@ class generic_gep_type_iterator {
if (auto *ATy = dyn_cast<ArrayType>(Ty))
CurTy = ATy->getElementType();
else if (auto *VTy = dyn_cast<VectorType>(Ty))
- CurTy = VTy->getElementType();
+ CurTy = VTy;
else
CurTy = dyn_cast<StructType>(Ty);
++OpIt;
@@ -108,7 +143,23 @@ class generic_gep_type_iterator {
// that.
bool isStruct() const { return isa<StructType *>(CurTy); }
- bool isSequential() const { return isa<Type *>(CurTy); }
+ bool isVector() const { return isa<VectorType *>(CurTy); }
+ bool isSequential() const { return !isStruct(); }
+
+ // For sequential GEP indices (all except those into structs), the index value
+ // can be translated into a byte offset by multiplying with an element stride.
+ // This function returns this stride, which both depends on the element type,
+ // and the containing aggregate type, as vectors always tightly bit-pack their
+ // elements.
+ TypeSize getSequentialElementStride(const DataLayout &DL) const {
+ assert(isSequential());
+ Type *ElemTy = getIndexedType();
+ TypeSize ElemSizeInBits = isVector() ? DL.getTypeSizeInBits(ElemTy)
+ : DL.getTypeAllocSizeInBits(ElemTy);
+ // Check for invalid GEPs that are not byte-addressable.
+ assert(ElemSizeInBits.isKnownMultipleOf(8));
+ return ElemSizeInBits.divideCoefficientBy(8);
+ }
StructType *getStructType() const { return cast<StructType *>(CurTy); }
diff --git a/llvm/lib/Analysis/BasicAliasAnalysis.cpp b/llvm/lib/Analysis/BasicAliasAnalysis.cpp
index 3de147368f2346..9eb063f0ee8674 100644
--- a/llvm/lib/Analysis/BasicAliasAnalysis.cpp
+++ b/llvm/lib/Analysis/BasicAliasAnalysis.cpp
@@ -639,7 +639,7 @@ BasicAAResult::DecomposeGEPExpression(const Value *V, const DataLayout &DL,
continue;
// Don't attempt to analyze GEPs if the scalable index is not zero.
- TypeSize AllocTypeSize = DL.getTypeAllocSize(GTI.getIndexedType());
+ TypeSize AllocTypeSize = GTI.getSequentialElementStride(DL);
if (AllocTypeSize.isScalable()) {
Decomposed.Base = V;
return Decomposed;
@@ -650,7 +650,7 @@ BasicAAResult::DecomposeGEPExpression(const Value *V, const DataLayout &DL,
continue;
}
- TypeSize AllocTypeSize = DL.getTypeAllocSize(GTI.getIndexedType());
+ TypeSize AllocTypeSize = GTI.getSequentialElementStride(DL);
if (AllocTypeSize.isScalable()) {
Decomposed.Base = V;
return Decomposed;
diff --git a/llvm/lib/Analysis/InlineCost.cpp b/llvm/lib/Analysis/InlineCost.cpp
index 7096e06d925ade..1fa7badaa4fa01 100644
--- a/llvm/lib/Analysis/InlineCost.cpp
+++ b/llvm/lib/Analysis/InlineCost.cpp
@@ -1429,7 +1429,7 @@ bool CallAnalyzer::accumulateGEPOffset(GEPOperator &GEP, APInt &Offset) {
continue;
}
- APInt TypeSize(IntPtrWidth, DL.getTypeAllocSize(GTI.getIndexedType()));
+ APInt TypeSize(IntPtrWidth, GTI.getSequentialElementStride(DL));
Offset += OpC->getValue().sextOrTrunc(IntPtrWidth) * TypeSize;
}
return true;
diff --git a/llvm/lib/Analysis/Local.cpp b/llvm/lib/Analysis/Local.cpp
index 30757abeb09802..f5e080d2c78e65 100644
--- a/llvm/lib/Analysis/Local.cpp
+++ b/llvm/lib/Analysis/Local.cpp
@@ -64,7 +64,7 @@ Value *llvm::emitGEPOffset(IRBuilderBase *Builder, const DataLayout &DL,
// Convert to correct type.
if (Op->getType() != IntIdxTy)
Op = Builder->CreateIntCast(Op, IntIdxTy, true, Op->getName() + ".c");
- TypeSize TSize = DL.getTypeAllocSize(GTI.getIndexedType());
+ TypeSize TSize = GTI.getSequentialElementStride(DL);
if (TSize != TypeSize::getFixed(1)) {
Value *Scale = Builder->CreateTypeSize(IntIdxTy->getScalarType(), TSize);
if (IntIdxTy->isVectorTy())
diff --git a/llvm/lib/Analysis/LoopAccessAnalysis.cpp b/llvm/lib/Analysis/LoopAccessAnalysis.cpp
index 0894560fd07898..ed22267ffe706b 100644
--- a/llvm/lib/Analysis/LoopAccessAnalysis.cpp
+++ b/llvm/lib/Analysis/LoopAccessAnalysis.cpp
@@ -2703,7 +2703,10 @@ static unsigned getGEPInductionOperand(const GetElementPtrInst *Gep) {
// If it's a type with the same allocation size as the result of the GEP we
// can peel off the zero index.
- if (DL.getTypeAllocSize(GEPTI.getIndexedType()) != GEPAllocSize)
+ TypeSize ElemSize = GEPTI.isStruct()
+ ? DL.getTypeAllocSize(GEPTI.getIndexedType())
+ : GEPTI.getSequentialElementStride(DL);
+ if (ElemSize != GEPAllocSize)
break;
--LastOperand;
}
diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp
index 5445746ab2a1bc..c0a3dd7926b577 100644
--- a/llvm/lib/Analysis/ValueTracking.cpp
+++ b/llvm/lib/Analysis/ValueTracking.cpp
@@ -1230,7 +1230,7 @@ static void computeKnownBitsFromOperator(const Operator *I,
unsigned IndexBitWidth = Index->getType()->getScalarSizeInBits();
KnownBits IndexBits(IndexBitWidth);
computeKnownBits(Index, IndexBits, Depth + 1, Q);
- TypeSize IndexTypeSize = Q.DL.getTypeAllocSize(IndexedTy);
+ TypeSize IndexTypeSize = GTI.getSequentialElementStride(Q.DL);
uint64_t TypeSizeInBytes = IndexTypeSize.getKnownMinValue();
KnownBits ScalingFactor(IndexBitWidth);
// Multiply by current sizeof type.
@@ -2158,7 +2158,7 @@ static bool isGEPKnownNonNull(const GEPOperator *GEP, unsigned Depth,
}
// If we have a zero-sized type, the index doesn't matter. Keep looping.
- if (Q.DL.getTypeAllocSize(GTI.getIndexedType()).isZero())
+ if (GTI.getSequentialElementStride(Q.DL).isZero())
continue;
// Fast path the constant operand case both for efficiency and so we don't
diff --git a/llvm/lib/CodeGen/CodeGenPrepare.cpp b/llvm/lib/CodeGen/CodeGenPrepare.cpp
index f9e791c7334838..57f3497f21f91c 100644
--- a/llvm/lib/CodeGen/CodeGenPrepare.cpp
+++ b/llvm/lib/CodeGen/CodeGenPrepare.cpp
@@ -4787,7 +4787,7 @@ bool AddressingModeMatcher::matchOperationAddr(User *AddrInst, unsigned Opcode,
cast<ConstantInt>(AddrInst->getOperand(i))->getZExtValue();
ConstantOffset += SL->getElementOffset(Idx);
} else {
- TypeSize TS = DL.getTypeAllocSize(GTI.getIndexedType());
+ TypeSize TS = GTI.getSequentialElementStride(DL);
if (TS.isNonZero()) {
// The optimisations below currently only work for fixed offsets.
if (TS.isScalable())
diff --git a/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp b/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
index 27a53e55f32fa3..97b47e74803633 100644
--- a/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
@@ -1545,7 +1545,7 @@ bool IRTranslator::translateGetElementPtr(const User &U,
Offset += DL->getStructLayout(StTy)->getElementOffset(Field);
continue;
} else {
- uint64_t ElementSize = DL->getTypeAllocSize(GTI.getIndexedType());
+ uint64_t ElementSize = GTI.getSequentialElementStride(*DL);
// If this is a scalar constant or a splat vector of constants,
// handle it quickly.
diff --git a/llvm/lib/CodeGen/SelectionDAG/FastISel.cpp b/llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
index a831295863399b..3f668c815ae1ae 100644
--- a/llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
@@ -560,15 +560,13 @@ bool FastISel::selectGetElementPtr(const User *I) {
}
}
} else {
- Type *Ty = GTI.getIndexedType();
-
// If this is a constant subscript, handle it quickly.
if (const auto *CI = dyn_cast<ConstantInt>(Idx)) {
if (CI->isZero())
continue;
// N = N + Offset
uint64_t IdxN = CI->getValue().sextOrTrunc(64).getSExtValue();
- TotalOffs += DL.getTypeAllocSize(Ty) * IdxN;
+ TotalOffs += GTI.getSequentialElementStride(DL) * IdxN;
if (TotalOffs >= MaxOffs) {
N = fastEmit_ri_(VT, ISD::ADD, N, TotalOffs, VT);
if (!N) // Unhandled operand. Halt "fast" selection and bail.
@@ -585,7 +583,7 @@ bool FastISel::selectGetElementPtr(const User *I) {
}
// N = N + Idx * ElementSize;
- uint64_t ElementSize = DL.getTypeAllocSize(Ty);
+ uint64_t ElementSize = GTI.getSequentialElementStride(DL);
Register IdxN = getRegForGEPIndex(Idx);
if (!IdxN) // Unhandled operand. Halt "fast" selection and bail.
return false;
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
index 12ed4a82ee91a5..6005bb92a7970d 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
@@ -4112,7 +4112,7 @@ void SelectionDAGBuilder::visitGetElementPtr(const User &I) {
unsigned IdxSize = DAG.getDataLayout().getIndexSizeInBits(AS);
MVT IdxTy = MVT::getIntegerVT(IdxSize);
TypeSize ElementSize =
- DAG.getDataLayout().getTypeAllocSize(GTI.getIndexedType());
+ GTI.getSequentialElementStride(DAG.getDataLayout());
// We intentionally mask away the high bits here; ElementSize may not
// fit in IdxTy.
APInt ElementMul(IdxSize, ElementSize.getKnownMinValue());
diff --git a/llvm/lib/ExecutionEngine/Interpreter/Execution.cpp b/llvm/lib/ExecutionEngine/Interpreter/Execution.cpp
index 770fc93490835d..ae978070ac9f90 100644
--- a/llvm/lib/ExecutionEngine/Interpreter/Execution.cpp
+++ b/llvm/lib/ExecutionEngine/Interpreter/Execution.cpp
@@ -1074,7 +1074,7 @@ GenericValue Interpreter::executeGEPOperation(Value *Ptr, gep_type_iterator I,
assert(BitWidth == 64 && "Invalid index type for getelementptr");
Idx = (int64_t)IdxGV.IntVal.getZExtValue();
}
- Total += getDataLayout().getTypeAllocSize(I.getIndexedType()) * Idx;
+ Total += I.getSequentialElementStride(getDataLayout()) * Idx;
}
}
diff --git a/llvm/lib/IR/DataLayout.cpp b/llvm/lib/IR/DataLayout.cpp
index e28f043cf9e0d0..a2f5714c706874 100644
--- a/llvm/lib/IR/DataLayout.cpp
+++ b/llvm/lib/IR/DataLayout.cpp
@@ -936,9 +936,8 @@ int64_t DataLayout::getIndexedOffsetInType(Type *ElemTy,
// Add in the offset, as calculated by the structure layout info...
Result += Layout->getElementOffset(FieldNo);
} else {
- // Get the array index and the size of each array element.
- if (int64_t arrayIdx = cast<ConstantInt>(Idx)->getSExtValue())
- Result += arrayIdx * getTypeAllocSize(GTI.getIndexedType());
+ if (int64_t ArrayIdx = cast<ConstantInt>(Idx)->getSExtValue())
+ Result += ArrayIdx * GTI.getSequentialElementStride(*this);
}
}
diff --git a/llvm/lib/IR/Operator.cpp b/llvm/lib/IR/Operator.cpp
index cd982c7da102af..16a89534b4b3ec 100644
--- a/llvm/lib/IR/Operator.cpp
+++ b/llvm/lib/IR/Operator.cpp
@@ -87,7 +87,7 @@ Align GEPOperator::getMaxPreservedAlignment(const DataLayout &DL) const {
/// If the index isn't known, we take 1 because it is the index that will
/// give the worse alignment of the offset.
const uint64_t ElemCount = OpC ? OpC->getZExtValue() : 1;
- Offset = DL.getTypeAllocSize(GTI.getIndexedType()) * ElemCount;
+ Offset = GTI.getSequentialElementStride(DL) * ElemCount;
}
Result = Align(MinAlign(Offset, Result.value()));
}
@@ -157,7 +157,7 @@ bool GEPOperator::accumulateConstantOffset(
continue;
}
if (!AccumulateOffset(ConstOffset->getValue(),
- DL.getTypeAllocSize(GTI.getIndexedType())))
+ GTI.getSequentialElementStride(DL)))
return false;
continue;
}
@@ -170,8 +170,7 @@ bool GEPOperator::accumulateConstantOffset(
if (!ExternalAnalysis(*V, AnalysisIndex))
return false;
UsedExternalAnalysis = true;
- if (!AccumulateOffset(AnalysisIndex,
- DL.getTypeAllocSize(GTI.getIndexedType())))
+ if (!AccumulateOffset(AnalysisIndex, GTI.getSequentialElementStride(DL)))
return false;
}
return true;
@@ -218,14 +217,13 @@ bool GEPOperator::collectOffset(
continue;
}
CollectConstantOffset(ConstOffset->getValue(),
- DL.getTypeAllocSize(GTI.getIndexedType()));
+ GTI.getSequentialElementStride(DL));
continue;
}
if (STy || ScalableType)
return false;
- APInt IndexedSize =
- APInt(BitWidth, DL.getTypeAllocSize(GTI.getIndexedType()));
+ APInt IndexedSize = APInt(BitWidth, GTI.getSequentialElementStride(DL));
// Insert an initial offset of 0 for V iff none exists already, then
// increment the offset by IndexedSize.
if (!IndexedSize.isZero()) {
diff --git a/llvm/lib/IR/Value.cpp b/llvm/lib/IR/Value.cpp
index b6e25c46b514d8..94b0ae7435c949 100644
--- a/llvm/lib/IR/Value.cpp
+++ b/llvm/lib/IR/Value.cpp
@@ -1015,7 +1015,7 @@ getOffsetFromIndex(const GEPOperator *GEP, unsigned Idx, const DataLayout &DL) {
// Otherwise, we have a sequential type like an array or fixed-length
// vector. Multiply the index by the ElementSize.
- TypeSize Size = DL.getTypeAllocSize(GTI.getIndexedType());
+ TypeSize Size = GTI.getSequentialElementStride(DL);
if (Size.isScalable())
return std::nullopt;
Offset += Size.getFixedValue() * OpC->getSExtValue();
diff --git a/llvm/lib/Target/AArch64/AArch64FastISel.cpp b/llvm/lib/Target/AArch64/AArch64FastISel.cpp
index 9b8162ce8dd4d0..fcb781e922084c 100644
--- a/llvm/lib/Target/AArch64/AArch64FastISel.cpp
+++ b/llvm/lib/Target/AArch64/AArch64FastISel.cpp
@@ -645,7 +645,7 @@ bool AArch64FastISel::computeAddress(const Value *Obj, Address &Addr, Type *Ty)
unsigned Idx = cast<ConstantInt>(Op)->getZExtValue();
TmpOffset += SL->getElementOffset(Idx);
} else {
- uint64_t S = DL.getTypeAllocSize(GTI.getIndexedType());
+ uint64_t S = GTI.getSequentialElementStride(DL);
while (true) {
if (const ConstantInt *CI = dyn_cast<ConstantInt>(Op)) {
// Constant-offset addressing.
@@ -4987,15 +4987,13 @@ bool AArch64FastISel::selectGetElementPtr(const Instruction *I) {
if (Field)
TotalOffs += DL.getStructLayout(StTy)->getElementOffset(Field);
} else {
- Type *Ty = GTI.getIndexedType();
-
// If this is a constant subscript, handle it quickly.
if (const auto *CI = dyn_cast<ConstantInt>(Idx)) {
if (CI->isZero())
continue;
// N = N + Offset
- TotalOffs +=
- DL.getTypeAllocSize(Ty) * cast<ConstantInt>(CI)->getSExtValue();
+ TotalOffs += GTI.getSequentialElementStride(DL) *
+ cast<ConstantInt>(CI)->getSExtValue();
continue;
}
if (TotalOffs) {
@@ -5006,7 +5004,7 @@ bool AArch64FastISel::selectGetElementPtr(const Instruction *I) {
}
// N = N + Idx * ElementSize;
- uint64_t ElementSize = DL.getTypeAllocSize(Ty);
+ uint64_t ElementSize = GTI.getSequentialElementStride(DL);
unsigned IdxN = getRegForGEPIndex(Idx);
if (!IdxN)
return false;
diff --git a/llvm/lib/Target/ARM/ARMFastISel.cpp b/llvm/lib/Target/ARM/ARMFastISel.cpp
index 1d6aaeb7433b0f..cb3a709f7003bd 100644
--- a/llvm/lib/Target/ARM/ARMFastISel.cpp
+++ b/llvm/lib/Target/ARM/ARMFastISel.cpp
@@ -747,7 +747,7 @@ bool ARMFastISel::ARMComputeAddress(const Value *Obj, Address &Addr) {
unsigned Idx = cast<ConstantInt>(Op)->getZExtValue();
TmpOffset += SL->getElementOffset(Idx);
} else {
- uint64_t S = DL.getTypeAllocSize(GTI.getIndexedType());
+ uint64_t S = GTI.getSequentialElementStride(DL);
while (true) {
if (const ConstantInt *CI = dyn_cast<ConstantInt>(Op)) {
// Constant-offset addressing.
diff --git a/llvm/lib/Target/Mips/MipsFastISel.cpp b/llvm/lib/Target/Mips/MipsFastISel.cpp
index 7fcf375aa10b69..192ed1cec79a84 100644
--- a/llvm/lib/Target/Mips/MipsFastISel.cpp
+++ b/llvm/lib/Target/Mips/MipsFastISel.cpp
@@ -492,7 +...
[truncated]
|
|
@llvm/pr-subscribers-clang Author: Jannik Silvanus (jasilvanus) ChangesVectors are always bit-packed and don't respect the elements' alignment This PR fixes many places that rely on an incorrect pattern This changes behavior for GEPs into vectors with element types for which the
Existing tests are unaffected, but a miscompilation of a new precommitted test is fixed. Patch is 28.39 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/75448.diff 28 Files Affected:
diff --git a/clang/lib/CodeGen/CGExprScalar.cpp b/clang/lib/CodeGen/CGExprScalar.cpp
index 41ad2ddac30d2d..f3eabd1ce224b8 100644
--- a/clang/lib/CodeGen/CGExprScalar.cpp
+++ b/clang/lib/CodeGen/CGExprScalar.cpp
@@ -5293,7 +5293,7 @@ static GEPOffsetAndOverflow EmitGEPOffsetInBytes(Value *BasePtr, Value *GEPVal,
// Otherwise this is array-like indexing. The local offset is the index
// multiplied by the element size.
auto *ElementSize = llvm::ConstantInt::get(
- IntPtrTy, DL.getTypeAllocSize(GTI.getIndexedType()));
+ IntPtrTy, GTI.getSequentialElementStride(DL)));
auto *IndexS = Builder.CreateIntCast(Index, IntPtrTy, /*isSigned=*/true);
LocalOffset = eval(BO_Mul, ElementSize, IndexS);
}
diff --git a/llvm/include/llvm/Analysis/TargetTransformInfoImpl.h b/llvm/include/llvm/Analysis/TargetTransformInfoImpl.h
index 1d8f523e9792ba..140838ff7c7c2d 100644
--- a/llvm/include/llvm/Analysis/TargetTransformInfoImpl.h
+++ b/llvm/include/llvm/Analysis/TargetTransformInfoImpl.h
@@ -1041,7 +1041,7 @@ class TargetTransformInfoImplCRTPBase : public TargetTransformInfoImplBase {
if (TargetType->isScalableTy())
return TTI::TCC_Basic;
int64_t ElementSize =
- DL.getTypeAllocSize(GTI.getIndexedType()).getFixedValue();
+ GTI.getSequentialElementStride(DL).getFixedValue();
if (ConstIdx) {
BaseOffset +=
ConstIdx->getValue().sextOrTrunc(PtrSizeBits) * ElementSize;
diff --git a/llvm/include/llvm/IR/GetElementPtrTypeIterator.h b/llvm/include/llvm/IR/GetElementPtrTypeIterator.h
index f3272327c3f8b2..5b63ccb182a842 100644
--- a/llvm/include/llvm/IR/GetElementPtrTypeIterator.h
+++ b/llvm/include/llvm/IR/GetElementPtrTypeIterator.h
@@ -16,6 +16,7 @@
#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/PointerUnion.h"
+#include "llvm/IR/DataLayout.h"
#include "llvm/IR/DerivedTypes.h"
#include "llvm/IR/Operator.h"
#include "llvm/IR/User.h"
@@ -30,7 +31,39 @@ template <typename ItTy = User::const_op_iterator>
class generic_gep_type_iterator {
ItTy OpIt;
- PointerUnion<StructType *, Type *> CurTy;
+ // We use two different mechanisms to store the type a GEP index applies to.
+ // In some cases, we need to know the outer aggregate type the index is
+ // applied within, e.g. a struct. In such cases, we store the aggregate type
+ // in the iterator, and derive the element type on the fly.
+ //
+ // However, this is not always possible, because for the outermost index there
+ // is no containing type. In such cases, or if the containing type is not
+ // relevant, e.g. for arrays, the element type is stored as Type* in CurTy.
+ //
+ // If CurTy contains a Type* value, this does not imply anything about the
+ // type itself, because it is the element type and not the outer type.
+ // In particular, Type* can be a struct type.
+ //
+ // Consider this example:
+ //
+ // %my.struct = type { i32, [ 4 x float ] }
+ // [...]
+ // %gep = getelementptr %my.struct, ptr %ptr, i32 10, i32 1, 32 3
+ //
+ // Iterating over the indices of this GEP, CurTy will contain the following
+ // values:
+ // * i32 10: The outer index always operates on the GEP value type.
+ // CurTy contains a Type* pointing at `%my.struct`.
+ // * i32 1: This index is within a struct.
+ // CurTy contains a StructType* pointing at `%my.struct`.
+ // * i32 3: This index is within an array. We reuse the "flat" indexing
+ // for arrays which is also used in the top level GEP index.
+ // CurTy contains a Type* pointing at `float`.
+ //
+ // Vectors are handled separately because the layout of vectors is different
+ // for overaligned elements: Vectors are always bit-packed, whereas arrays
+ // respect ABI alignment of the elements.
+ PointerUnion<StructType *, VectorType *, Type *> CurTy;
generic_gep_type_iterator() = default;
@@ -69,6 +102,8 @@ class generic_gep_type_iterator {
Type *getIndexedType() const {
if (auto *T = dyn_cast_if_present<Type *>(CurTy))
return T;
+ if (auto *VT = dyn_cast_if_present<VectorType *>(CurTy))
+ return VT->getElementType();
return cast<StructType *>(CurTy)->getTypeAtIndex(getOperand());
}
@@ -79,7 +114,7 @@ class generic_gep_type_iterator {
if (auto *ATy = dyn_cast<ArrayType>(Ty))
CurTy = ATy->getElementType();
else if (auto *VTy = dyn_cast<VectorType>(Ty))
- CurTy = VTy->getElementType();
+ CurTy = VTy;
else
CurTy = dyn_cast<StructType>(Ty);
++OpIt;
@@ -108,7 +143,23 @@ class generic_gep_type_iterator {
// that.
bool isStruct() const { return isa<StructType *>(CurTy); }
- bool isSequential() const { return isa<Type *>(CurTy); }
+ bool isVector() const { return isa<VectorType *>(CurTy); }
+ bool isSequential() const { return !isStruct(); }
+
+ // For sequential GEP indices (all except those into structs), the index value
+ // can be translated into a byte offset by multiplying with an element stride.
+ // This function returns this stride, which both depends on the element type,
+ // and the containing aggregate type, as vectors always tightly bit-pack their
+ // elements.
+ TypeSize getSequentialElementStride(const DataLayout &DL) const {
+ assert(isSequential());
+ Type *ElemTy = getIndexedType();
+ TypeSize ElemSizeInBits = isVector() ? DL.getTypeSizeInBits(ElemTy)
+ : DL.getTypeAllocSizeInBits(ElemTy);
+ // Check for invalid GEPs that are not byte-addressable.
+ assert(ElemSizeInBits.isKnownMultipleOf(8));
+ return ElemSizeInBits.divideCoefficientBy(8);
+ }
StructType *getStructType() const { return cast<StructType *>(CurTy); }
diff --git a/llvm/lib/Analysis/BasicAliasAnalysis.cpp b/llvm/lib/Analysis/BasicAliasAnalysis.cpp
index 3de147368f2346..9eb063f0ee8674 100644
--- a/llvm/lib/Analysis/BasicAliasAnalysis.cpp
+++ b/llvm/lib/Analysis/BasicAliasAnalysis.cpp
@@ -639,7 +639,7 @@ BasicAAResult::DecomposeGEPExpression(const Value *V, const DataLayout &DL,
continue;
// Don't attempt to analyze GEPs if the scalable index is not zero.
- TypeSize AllocTypeSize = DL.getTypeAllocSize(GTI.getIndexedType());
+ TypeSize AllocTypeSize = GTI.getSequentialElementStride(DL);
if (AllocTypeSize.isScalable()) {
Decomposed.Base = V;
return Decomposed;
@@ -650,7 +650,7 @@ BasicAAResult::DecomposeGEPExpression(const Value *V, const DataLayout &DL,
continue;
}
- TypeSize AllocTypeSize = DL.getTypeAllocSize(GTI.getIndexedType());
+ TypeSize AllocTypeSize = GTI.getSequentialElementStride(DL);
if (AllocTypeSize.isScalable()) {
Decomposed.Base = V;
return Decomposed;
diff --git a/llvm/lib/Analysis/InlineCost.cpp b/llvm/lib/Analysis/InlineCost.cpp
index 7096e06d925ade..1fa7badaa4fa01 100644
--- a/llvm/lib/Analysis/InlineCost.cpp
+++ b/llvm/lib/Analysis/InlineCost.cpp
@@ -1429,7 +1429,7 @@ bool CallAnalyzer::accumulateGEPOffset(GEPOperator &GEP, APInt &Offset) {
continue;
}
- APInt TypeSize(IntPtrWidth, DL.getTypeAllocSize(GTI.getIndexedType()));
+ APInt TypeSize(IntPtrWidth, GTI.getSequentialElementStride(DL));
Offset += OpC->getValue().sextOrTrunc(IntPtrWidth) * TypeSize;
}
return true;
diff --git a/llvm/lib/Analysis/Local.cpp b/llvm/lib/Analysis/Local.cpp
index 30757abeb09802..f5e080d2c78e65 100644
--- a/llvm/lib/Analysis/Local.cpp
+++ b/llvm/lib/Analysis/Local.cpp
@@ -64,7 +64,7 @@ Value *llvm::emitGEPOffset(IRBuilderBase *Builder, const DataLayout &DL,
// Convert to correct type.
if (Op->getType() != IntIdxTy)
Op = Builder->CreateIntCast(Op, IntIdxTy, true, Op->getName() + ".c");
- TypeSize TSize = DL.getTypeAllocSize(GTI.getIndexedType());
+ TypeSize TSize = GTI.getSequentialElementStride(DL);
if (TSize != TypeSize::getFixed(1)) {
Value *Scale = Builder->CreateTypeSize(IntIdxTy->getScalarType(), TSize);
if (IntIdxTy->isVectorTy())
diff --git a/llvm/lib/Analysis/LoopAccessAnalysis.cpp b/llvm/lib/Analysis/LoopAccessAnalysis.cpp
index 0894560fd07898..ed22267ffe706b 100644
--- a/llvm/lib/Analysis/LoopAccessAnalysis.cpp
+++ b/llvm/lib/Analysis/LoopAccessAnalysis.cpp
@@ -2703,7 +2703,10 @@ static unsigned getGEPInductionOperand(const GetElementPtrInst *Gep) {
// If it's a type with the same allocation size as the result of the GEP we
// can peel off the zero index.
- if (DL.getTypeAllocSize(GEPTI.getIndexedType()) != GEPAllocSize)
+ TypeSize ElemSize = GEPTI.isStruct()
+ ? DL.getTypeAllocSize(GEPTI.getIndexedType())
+ : GEPTI.getSequentialElementStride(DL);
+ if (ElemSize != GEPAllocSize)
break;
--LastOperand;
}
diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp
index 5445746ab2a1bc..c0a3dd7926b577 100644
--- a/llvm/lib/Analysis/ValueTracking.cpp
+++ b/llvm/lib/Analysis/ValueTracking.cpp
@@ -1230,7 +1230,7 @@ static void computeKnownBitsFromOperator(const Operator *I,
unsigned IndexBitWidth = Index->getType()->getScalarSizeInBits();
KnownBits IndexBits(IndexBitWidth);
computeKnownBits(Index, IndexBits, Depth + 1, Q);
- TypeSize IndexTypeSize = Q.DL.getTypeAllocSize(IndexedTy);
+ TypeSize IndexTypeSize = GTI.getSequentialElementStride(Q.DL);
uint64_t TypeSizeInBytes = IndexTypeSize.getKnownMinValue();
KnownBits ScalingFactor(IndexBitWidth);
// Multiply by current sizeof type.
@@ -2158,7 +2158,7 @@ static bool isGEPKnownNonNull(const GEPOperator *GEP, unsigned Depth,
}
// If we have a zero-sized type, the index doesn't matter. Keep looping.
- if (Q.DL.getTypeAllocSize(GTI.getIndexedType()).isZero())
+ if (GTI.getSequentialElementStride(Q.DL).isZero())
continue;
// Fast path the constant operand case both for efficiency and so we don't
diff --git a/llvm/lib/CodeGen/CodeGenPrepare.cpp b/llvm/lib/CodeGen/CodeGenPrepare.cpp
index f9e791c7334838..57f3497f21f91c 100644
--- a/llvm/lib/CodeGen/CodeGenPrepare.cpp
+++ b/llvm/lib/CodeGen/CodeGenPrepare.cpp
@@ -4787,7 +4787,7 @@ bool AddressingModeMatcher::matchOperationAddr(User *AddrInst, unsigned Opcode,
cast<ConstantInt>(AddrInst->getOperand(i))->getZExtValue();
ConstantOffset += SL->getElementOffset(Idx);
} else {
- TypeSize TS = DL.getTypeAllocSize(GTI.getIndexedType());
+ TypeSize TS = GTI.getSequentialElementStride(DL);
if (TS.isNonZero()) {
// The optimisations below currently only work for fixed offsets.
if (TS.isScalable())
diff --git a/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp b/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
index 27a53e55f32fa3..97b47e74803633 100644
--- a/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
@@ -1545,7 +1545,7 @@ bool IRTranslator::translateGetElementPtr(const User &U,
Offset += DL->getStructLayout(StTy)->getElementOffset(Field);
continue;
} else {
- uint64_t ElementSize = DL->getTypeAllocSize(GTI.getIndexedType());
+ uint64_t ElementSize = GTI.getSequentialElementStride(*DL);
// If this is a scalar constant or a splat vector of constants,
// handle it quickly.
diff --git a/llvm/lib/CodeGen/SelectionDAG/FastISel.cpp b/llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
index a831295863399b..3f668c815ae1ae 100644
--- a/llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
@@ -560,15 +560,13 @@ bool FastISel::selectGetElementPtr(const User *I) {
}
}
} else {
- Type *Ty = GTI.getIndexedType();
-
// If this is a constant subscript, handle it quickly.
if (const auto *CI = dyn_cast<ConstantInt>(Idx)) {
if (CI->isZero())
continue;
// N = N + Offset
uint64_t IdxN = CI->getValue().sextOrTrunc(64).getSExtValue();
- TotalOffs += DL.getTypeAllocSize(Ty) * IdxN;
+ TotalOffs += GTI.getSequentialElementStride(DL) * IdxN;
if (TotalOffs >= MaxOffs) {
N = fastEmit_ri_(VT, ISD::ADD, N, TotalOffs, VT);
if (!N) // Unhandled operand. Halt "fast" selection and bail.
@@ -585,7 +583,7 @@ bool FastISel::selectGetElementPtr(const User *I) {
}
// N = N + Idx * ElementSize;
- uint64_t ElementSize = DL.getTypeAllocSize(Ty);
+ uint64_t ElementSize = GTI.getSequentialElementStride(DL);
Register IdxN = getRegForGEPIndex(Idx);
if (!IdxN) // Unhandled operand. Halt "fast" selection and bail.
return false;
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
index 12ed4a82ee91a5..6005bb92a7970d 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
@@ -4112,7 +4112,7 @@ void SelectionDAGBuilder::visitGetElementPtr(const User &I) {
unsigned IdxSize = DAG.getDataLayout().getIndexSizeInBits(AS);
MVT IdxTy = MVT::getIntegerVT(IdxSize);
TypeSize ElementSize =
- DAG.getDataLayout().getTypeAllocSize(GTI.getIndexedType());
+ GTI.getSequentialElementStride(DAG.getDataLayout());
// We intentionally mask away the high bits here; ElementSize may not
// fit in IdxTy.
APInt ElementMul(IdxSize, ElementSize.getKnownMinValue());
diff --git a/llvm/lib/ExecutionEngine/Interpreter/Execution.cpp b/llvm/lib/ExecutionEngine/Interpreter/Execution.cpp
index 770fc93490835d..ae978070ac9f90 100644
--- a/llvm/lib/ExecutionEngine/Interpreter/Execution.cpp
+++ b/llvm/lib/ExecutionEngine/Interpreter/Execution.cpp
@@ -1074,7 +1074,7 @@ GenericValue Interpreter::executeGEPOperation(Value *Ptr, gep_type_iterator I,
assert(BitWidth == 64 && "Invalid index type for getelementptr");
Idx = (int64_t)IdxGV.IntVal.getZExtValue();
}
- Total += getDataLayout().getTypeAllocSize(I.getIndexedType()) * Idx;
+ Total += I.getSequentialElementStride(getDataLayout()) * Idx;
}
}
diff --git a/llvm/lib/IR/DataLayout.cpp b/llvm/lib/IR/DataLayout.cpp
index e28f043cf9e0d0..a2f5714c706874 100644
--- a/llvm/lib/IR/DataLayout.cpp
+++ b/llvm/lib/IR/DataLayout.cpp
@@ -936,9 +936,8 @@ int64_t DataLayout::getIndexedOffsetInType(Type *ElemTy,
// Add in the offset, as calculated by the structure layout info...
Result += Layout->getElementOffset(FieldNo);
} else {
- // Get the array index and the size of each array element.
- if (int64_t arrayIdx = cast<ConstantInt>(Idx)->getSExtValue())
- Result += arrayIdx * getTypeAllocSize(GTI.getIndexedType());
+ if (int64_t ArrayIdx = cast<ConstantInt>(Idx)->getSExtValue())
+ Result += ArrayIdx * GTI.getSequentialElementStride(*this);
}
}
diff --git a/llvm/lib/IR/Operator.cpp b/llvm/lib/IR/Operator.cpp
index cd982c7da102af..16a89534b4b3ec 100644
--- a/llvm/lib/IR/Operator.cpp
+++ b/llvm/lib/IR/Operator.cpp
@@ -87,7 +87,7 @@ Align GEPOperator::getMaxPreservedAlignment(const DataLayout &DL) const {
/// If the index isn't known, we take 1 because it is the index that will
/// give the worse alignment of the offset.
const uint64_t ElemCount = OpC ? OpC->getZExtValue() : 1;
- Offset = DL.getTypeAllocSize(GTI.getIndexedType()) * ElemCount;
+ Offset = GTI.getSequentialElementStride(DL) * ElemCount;
}
Result = Align(MinAlign(Offset, Result.value()));
}
@@ -157,7 +157,7 @@ bool GEPOperator::accumulateConstantOffset(
continue;
}
if (!AccumulateOffset(ConstOffset->getValue(),
- DL.getTypeAllocSize(GTI.getIndexedType())))
+ GTI.getSequentialElementStride(DL)))
return false;
continue;
}
@@ -170,8 +170,7 @@ bool GEPOperator::accumulateConstantOffset(
if (!ExternalAnalysis(*V, AnalysisIndex))
return false;
UsedExternalAnalysis = true;
- if (!AccumulateOffset(AnalysisIndex,
- DL.getTypeAllocSize(GTI.getIndexedType())))
+ if (!AccumulateOffset(AnalysisIndex, GTI.getSequentialElementStride(DL)))
return false;
}
return true;
@@ -218,14 +217,13 @@ bool GEPOperator::collectOffset(
continue;
}
CollectConstantOffset(ConstOffset->getValue(),
- DL.getTypeAllocSize(GTI.getIndexedType()));
+ GTI.getSequentialElementStride(DL));
continue;
}
if (STy || ScalableType)
return false;
- APInt IndexedSize =
- APInt(BitWidth, DL.getTypeAllocSize(GTI.getIndexedType()));
+ APInt IndexedSize = APInt(BitWidth, GTI.getSequentialElementStride(DL));
// Insert an initial offset of 0 for V iff none exists already, then
// increment the offset by IndexedSize.
if (!IndexedSize.isZero()) {
diff --git a/llvm/lib/IR/Value.cpp b/llvm/lib/IR/Value.cpp
index b6e25c46b514d8..94b0ae7435c949 100644
--- a/llvm/lib/IR/Value.cpp
+++ b/llvm/lib/IR/Value.cpp
@@ -1015,7 +1015,7 @@ getOffsetFromIndex(const GEPOperator *GEP, unsigned Idx, const DataLayout &DL) {
// Otherwise, we have a sequential type like an array or fixed-length
// vector. Multiply the index by the ElementSize.
- TypeSize Size = DL.getTypeAllocSize(GTI.getIndexedType());
+ TypeSize Size = GTI.getSequentialElementStride(DL);
if (Size.isScalable())
return std::nullopt;
Offset += Size.getFixedValue() * OpC->getSExtValue();
diff --git a/llvm/lib/Target/AArch64/AArch64FastISel.cpp b/llvm/lib/Target/AArch64/AArch64FastISel.cpp
index 9b8162ce8dd4d0..fcb781e922084c 100644
--- a/llvm/lib/Target/AArch64/AArch64FastISel.cpp
+++ b/llvm/lib/Target/AArch64/AArch64FastISel.cpp
@@ -645,7 +645,7 @@ bool AArch64FastISel::computeAddress(const Value *Obj, Address &Addr, Type *Ty)
unsigned Idx = cast<ConstantInt>(Op)->getZExtValue();
TmpOffset += SL->getElementOffset(Idx);
} else {
- uint64_t S = DL.getTypeAllocSize(GTI.getIndexedType());
+ uint64_t S = GTI.getSequentialElementStride(DL);
while (true) {
if (const ConstantInt *CI = dyn_cast<ConstantInt>(Op)) {
// Constant-offset addressing.
@@ -4987,15 +4987,13 @@ bool AArch64FastISel::selectGetElementPtr(const Instruction *I) {
if (Field)
TotalOffs += DL.getStructLayout(StTy)->getElementOffset(Field);
} else {
- Type *Ty = GTI.getIndexedType();
-
// If this is a constant subscript, handle it quickly.
if (const auto *CI = dyn_cast<ConstantInt>(Idx)) {
if (CI->isZero())
continue;
// N = N + Offset
- TotalOffs +=
- DL.getTypeAllocSize(Ty) * cast<ConstantInt>(CI)->getSExtValue();
+ TotalOffs += GTI.getSequentialElementStride(DL) *
+ cast<ConstantInt>(CI)->getSExtValue();
continue;
}
if (TotalOffs) {
@@ -5006,7 +5004,7 @@ bool AArch64FastISel::selectGetElementPtr(const Instruction *I) {
}
// N = N + Idx * ElementSize;
- uint64_t ElementSize = DL.getTypeAllocSize(Ty);
+ uint64_t ElementSize = GTI.getSequentialElementStride(DL);
unsigned IdxN = getRegForGEPIndex(Idx);
if (!IdxN)
return false;
diff --git a/llvm/lib/Target/ARM/ARMFastISel.cpp b/llvm/lib/Target/ARM/ARMFastISel.cpp
index 1d6aaeb7433b0f..cb3a709f7003bd 100644
--- a/llvm/lib/Target/ARM/ARMFastISel.cpp
+++ b/llvm/lib/Target/ARM/ARMFastISel.cpp
@@ -747,7 +747,7 @@ bool ARMFastISel::ARMComputeAddress(const Value *Obj, Address &Addr) {
unsigned Idx = cast<ConstantInt>(Op)->getZExtValue();
TmpOffset += SL->getElementOffset(Idx);
} else {
- uint64_t S = DL.getTypeAllocSize(GTI.getIndexedType());
+ uint64_t S = GTI.getSequentialElementStride(DL);
while (true) {
if (const ConstantInt *CI = dyn_cast<ConstantInt>(Op)) {
// Constant-offset addressing.
diff --git a/llvm/lib/Target/Mips/MipsFastISel.cpp b/llvm/lib/Target/Mips/MipsFastISel.cpp
index 7fcf375aa10b69..192ed1cec79a84 100644
--- a/llvm/lib/Target/Mips/MipsFastISel.cpp
+++ b/llvm/lib/Target/Mips/MipsFastISel.cpp
@@ -492,7 +...
[truncated]
|
|
Can this solve #68566 too? |
jasilvanus
force-pushed
the
jsilvanu/gep-collect-offsets-overaligned-vector
branch
from
294fac0 to
4649d44
Compare
|
Vectors are always bit-packed and don't respect the elements' alignment requirements. This is different from arrays. This means offsets of GEPs into vectors need to be computed differently than array GEPs. This patch fixes many places by replacing a common-but-incorrect pattern that always relies on DL.getTypeAllocSize() by GTI.getSequentialElementStride(DL). This changes behavior for GEPs into vectors with element types that have a (bit) size and alloc size. This includes two cases: * Types with a bit size that is not a multiple of a byte, e.g. i1. GEPs into such vectors are questionable to begin with, as some elements are not even addressable. * Overaligned types, e.g. i16 with 32-bit alignment. Existing tests are unaffected, except for one precommitted test that is no longer miscompiled.
jasilvanus
force-pushed
the
jsilvanu/gep-collect-offsets-overaligned-vector
branch
from
4649d44 to
e69482f
Compare
I don't think it solves it, as it only fixes offset computations within GEPs and doesn't teach code in general about the correct vector layout. However, it is reducing the amount of code assuming the wrong layout. There seem to be some clang test failures though that I'm currently looking into. |
Solved: The clang failures were caused by me having Clang was invoking |
nikic
reviewed
Dec 14, 2023
Co-authored-by: Nikita Popov <github@npopov.com>
I agree that it would be better if there just were no vector GEPs, but that breaks importing older modules, and that cannot be easily auto-upgraded (convert to byte-GEPs?). Even worse, DXIL uses different vector semantics where alignment in vectors is respected, so importing DXIL already is a challenge, but if vector GEPs in DXIL were to be replaced by byte-GEPs as part of auto-upgrade, preserving DXIL semantics becomes difficult. |
|
I'm now on vacation for two weeks, returning Jan 4th. |
Yes, upgrade would be to byte-GEPs.
Would probably need a bitcode parser hook for the conversion of GEP indices to offsets. You'll have to deal with this problem at some point anyway, once we start upgrading all GEPs to byte-GEPs in the not-so-distant future. |
nikic
approved these changes
Dec 22, 2023
|
Alive2 is complaining about one of the tests: @Global = global 10 bytes, align 1
define void @test_overaligned_vec(i8 %B) {
%A = gep ptr @Global, 4 x i64 0, 4 x i64 1
store i8 %B, ptr %A, align 1
ret void
}
=>
@Global = global 10 bytes, align 1
define void @test_overaligned_vec(i8 %B) {
%__constexpr_0 = gep inbounds ptr @Global, 10 x i64 0, 1 x i64 2
store i8 %B, ptr %__constexpr_0, align 1
ret void
}
Transformation doesn't verify! (unsound)
ERROR: Mismatch in memory
Example:
i8 %B = #x11 (17)
Source:
ptr %A = pointer(non-local, block_id=0, offset=4)GEP of 4 vs 2 bytes. |
|
Interesting, here it is online: https://alive2.llvm.org/ce/z/Bm5gP2 Looks like an Alive2 bug with respect to vector bit layout? Edit: I suspect this piece needs to be fixed similar to the changes in this PR: https://github.com/AliveToolkit/alive2/blob/64fd5fb408a12439daed663a1136a8409f361261/llvm_util/llvm2alive.cpp#L631 Edit: Opened AliveToolkit/alive2#1002 to track this. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Vectors are always bit-packed and don't respect the elements' alignment
requirements. This is different from arrays. This means offsets of vector GEPs
need to be computed differently than offsets of array GEPs.
This PR fixes many places that rely on an incorrect pattern
that always relies on
DL.getTypeAllocSize(GTI.getIndexedType()).We replace these by usages of
GTI.getSequentialElementStride(DL),which is a new helper function added in this PR.
This changes behavior for GEPs into vectors with element types for which the
(bit) size and alloc size is different. This includes two cases:
GEPs into such vectors are questionable to begin with, as some elements
are not even addressable.
Existing tests are unaffected, but a miscompilation of a new precommitted test is fixed.