[delinearize] Extract array dimensions from alloca and global declarations #156342
Conversation
llvmbot
added
llvm:analysis
|
@llvm/pr-subscribers-llvm-analysis @llvm/pr-subscribers-llvm-transforms Author: Sebastian Pop (sebpop) ChangesThis implements an alternative to extract multi-dimensional array information The functionality for GEP delinearization remains in this patch such that we can Next patch removes the code for GEP delinearization. Patch is 227.84 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/156342.diff 92 Files Affected:
diff --git a/llvm/include/llvm/Analysis/Delinearization.h b/llvm/include/llvm/Analysis/Delinearization.h
index 434cfb61699d6..0dafac9102247 100644
--- a/llvm/include/llvm/Analysis/Delinearization.h
+++ b/llvm/include/llvm/Analysis/Delinearization.h
@@ -112,6 +112,13 @@ void delinearize(ScalarEvolution &SE, const SCEV *Expr,
SmallVectorImpl<const SCEV *> &Subscripts,
SmallVectorImpl<const SCEV *> &Sizes, const SCEV *ElementSize);
+/// Same as delinearize. TODO: only use delinearize() instead of other internal
+/// functions.
+bool delinearizeUsingArrayInfo(ScalarEvolution &SE, const SCEV *AccessFn,
+ SmallVectorImpl<const SCEV *> &Subscripts,
+ SmallVectorImpl<const SCEV *> &Sizes,
+ const SCEV *ElementSize);
+
/// Compute the dimensions of fixed size array from \Expr and save the results
/// in \p Sizes.
bool findFixedSizeArrayDimensions(ScalarEvolution &SE, const SCEV *Expr,
@@ -155,6 +162,17 @@ bool getIndexExpressionsFromGEP(ScalarEvolution &SE,
SmallVectorImpl<const SCEV *> &Subscripts,
SmallVectorImpl<int> &Sizes);
+/// Compute access functions for each subscript in a delinearized array access.
+void computeAccessFunctions(ScalarEvolution &SE, const SCEV *Expr,
+ SmallVectorImpl<const SCEV *> &Subscripts,
+ SmallVectorImpl<const SCEV *> &Sizes,
+ const SCEV *ElementSize);
+
+/// Backward compatibility wrapper for computeAccessFunctions.
+void computeAccessFunctions(ScalarEvolution &SE, const SCEV *Expr,
+ SmallVectorImpl<const SCEV *> &Subscripts,
+ SmallVectorImpl<const SCEV *> &Sizes);
+
/// Implementation of fixed size array delinearization. Try to delinearize
/// access function for a fixed size multi-dimensional array, by deriving
/// subscripts from GEP instructions. Returns true upon success and false
diff --git a/llvm/lib/Analysis/Delinearization.cpp b/llvm/lib/Analysis/Delinearization.cpp
index 762d9191aab1e..1d07d632fe787 100644
--- a/llvm/lib/Analysis/Delinearization.cpp
+++ b/llvm/lib/Analysis/Delinearization.cpp
@@ -14,15 +14,18 @@
//===----------------------------------------------------------------------===//
#include "llvm/Analysis/Delinearization.h"
+#include "llvm/ADT/DenseMap.h"
#include "llvm/Analysis/LoopInfo.h"
#include "llvm/Analysis/ScalarEvolution.h"
#include "llvm/Analysis/ScalarEvolutionDivision.h"
#include "llvm/Analysis/ScalarEvolutionExpressions.h"
#include "llvm/IR/Constants.h"
+#include "llvm/IR/DataLayout.h"
#include "llvm/IR/DerivedTypes.h"
#include "llvm/IR/Function.h"
#include "llvm/IR/InstIterator.h"
#include "llvm/IR/Instructions.h"
+#include "llvm/IR/Module.h"
#include "llvm/IR/PassManager.h"
#include "llvm/Support/CommandLine.h"
#include "llvm/Support/Debug.h"
@@ -38,6 +41,10 @@ static cl::opt<bool> UseFixedSizeArrayHeuristic(
cl::desc("When printing analysis, use the heuristic for fixed-size arrays "
"if the default delinearizetion fails."));
+static cl::opt<bool> useGEPToDelinearize(
+ "use-gep-to-delinearize", cl::init(true), cl::Hidden,
+ cl::desc("validate both delinearization methods match."));
+
// Return true when S contains at least an undef value.
static inline bool containsUndefs(const SCEV *S) {
return SCEVExprContains(S, [](const SCEV *S) {
@@ -182,7 +189,7 @@ void llvm::collectParametricTerms(ScalarEvolution &SE, const SCEV *Expr,
LLVM_DEBUG({
dbgs() << "Strides:\n";
for (const SCEV *S : Strides)
- dbgs() << *S << "\n";
+ dbgs() << " " << *S << "\n";
});
for (const SCEV *S : Strides) {
@@ -193,7 +200,7 @@ void llvm::collectParametricTerms(ScalarEvolution &SE, const SCEV *Expr,
LLVM_DEBUG({
dbgs() << "Terms:\n";
for (const SCEV *T : Terms)
- dbgs() << *T << "\n";
+ dbgs() << " " << *T << "\n";
});
SCEVCollectAddRecMultiplies MulCollector(Terms, SE);
@@ -294,7 +301,7 @@ void llvm::findArrayDimensions(ScalarEvolution &SE,
LLVM_DEBUG({
dbgs() << "Terms:\n";
for (const SCEV *T : Terms)
- dbgs() << *T << "\n";
+ dbgs() << " " << *T << "\n";
});
// Remove duplicates.
@@ -325,7 +332,7 @@ void llvm::findArrayDimensions(ScalarEvolution &SE,
LLVM_DEBUG({
dbgs() << "Terms after sorting:\n";
for (const SCEV *T : NewTerms)
- dbgs() << *T << "\n";
+ dbgs() << " " << *T << "\n";
});
if (NewTerms.empty() || !findArrayDimensionsRec(SE, NewTerms, Sizes)) {
@@ -339,13 +346,14 @@ void llvm::findArrayDimensions(ScalarEvolution &SE,
LLVM_DEBUG({
dbgs() << "Sizes:\n";
for (const SCEV *S : Sizes)
- dbgs() << *S << "\n";
+ dbgs() << " " << *S << "\n";
});
}
void llvm::computeAccessFunctions(ScalarEvolution &SE, const SCEV *Expr,
SmallVectorImpl<const SCEV *> &Subscripts,
- SmallVectorImpl<const SCEV *> &Sizes) {
+ SmallVectorImpl<const SCEV *> &Sizes,
+ const SCEV *ElementSize) {
// Early exit in case this SCEV is not an affine multivariate function.
if (Sizes.empty())
return;
@@ -354,20 +362,32 @@ void llvm::computeAccessFunctions(ScalarEvolution &SE, const SCEV *Expr,
if (!AR->isAffine())
return;
+ if (ElementSize->isZero())
+ return;
+
+ // Clear output vector.
+ Subscripts.clear();
+
+ LLVM_DEBUG(dbgs() << "\ncomputeAccessFunctions\n"
+ << "Linearized Memory Access Function: " << *Expr << "\n");
+
const SCEV *Res = Expr;
int Last = Sizes.size() - 1;
+
for (int i = Last; i >= 0; i--) {
- const SCEV *Q, *R;
- SCEVDivision::divide(SE, Res, Sizes[i], &Q, &R);
+ const SCEV *Size = Sizes[i];
+ if (Size->isZero())
+ continue;
+ const SCEV *Q, *R;
+ SCEVDivision::divide(SE, Res, Size, &Q, &R);
LLVM_DEBUG({
- dbgs() << "Res: " << *Res << "\n";
- dbgs() << "Sizes[i]: " << *Sizes[i] << "\n";
- dbgs() << "Res divided by Sizes[i]:\n";
- dbgs() << "Quotient: " << *Q << "\n";
- dbgs() << "Remainder: " << *R << "\n";
+ dbgs() << "Computing 'MemAccFn / Sizes[" << i << "]':\n";
+ dbgs() << " MemAccFn: " << *Res << "\n";
+ dbgs() << " Sizes[" << i << "]: " << *Size << "\n";
+ dbgs() << " Quotient (Leftover): " << *Q << "\n";
+ dbgs() << " Remainder (Subscript Access Function): " << *R << "\n";
});
-
Res = Q;
// Do not record the last subscript corresponding to the size of elements in
@@ -385,22 +405,169 @@ void llvm::computeAccessFunctions(ScalarEvolution &SE, const SCEV *Expr,
}
// Record the access function for the current subscript.
+ LLVM_DEBUG(dbgs() << "Subscripts push_back Remainder: " << *R << "\n");
Subscripts.push_back(R);
}
- // Also push in last position the remainder of the last division: it will be
- // the access function of the innermost dimension.
- Subscripts.push_back(Res);
+ // Also push in last position the quotient "Res = Q" of the last division: it
+ // will be the access function of the outermost array dimension.
+ if (!Res->isZero()) {
+ // This is only needed when the outermost array size is not known. Res = 0
+ // when the outermost array dimension is known, as for example when reading
+ // array sizes from a local or global declaration.
+ Subscripts.push_back(Res);
+ LLVM_DEBUG(dbgs() << "Subscripts push_back Res: " << *Res << "\n");
+ }
std::reverse(Subscripts.begin(), Subscripts.end());
LLVM_DEBUG({
dbgs() << "Subscripts:\n";
for (const SCEV *S : Subscripts)
- dbgs() << *S << "\n";
+ dbgs() << " " << *S << "\n";
+ dbgs() << "\n";
});
}
+/// Backward compatibility wrapper for the old 4-parameter version.
+void llvm::computeAccessFunctions(ScalarEvolution &SE, const SCEV *Expr,
+ SmallVectorImpl<const SCEV *> &Subscripts,
+ SmallVectorImpl<const SCEV *> &Sizes) {
+ // Use the element size from the last element in Sizes array (legacy behavior)
+ if (Sizes.empty()) {
+ Subscripts.clear();
+ return;
+ }
+ const SCEV *ElementSize = Sizes.back();
+ computeAccessFunctions(SE, Expr, Subscripts, Sizes, ElementSize);
+}
+
+/// Extract array dimensions from alloca or global variable declarations.
+/// Returns true if array dimensions were successfully extracted.
+static bool
+extractArrayInfoFromAllocaOrGlobal(ScalarEvolution &SE, Value *BasePtr,
+ SmallVectorImpl<const SCEV *> &Sizes,
+ const SCEV *ElementSize) {
+ // Clear output vector.
+ Sizes.clear();
+
+ LLVM_DEBUG(
+ dbgs() << "extractArrayInfoFromAllocaOrGlobal called with BasePtr: "
+ << *BasePtr << "\n");
+
+ // Distinguish between simple array accesses and complex pointer arithmetic.
+ // Only apply array_info extraction to direct array accesses to avoid
+ // incorrect delinearization of complex pointer arithmetic patterns.
+ if (auto *GEP = dyn_cast<GetElementPtrInst>(BasePtr)) {
+ // Check if this is a simple array access pattern: GEP [N x T]* @array, 0,
+ // idx This represents direct indexing like array[i], which should use array
+ // dimensions.
+ if (GEP->getNumIndices() == 2) {
+ auto *FirstIdx = dyn_cast<ConstantInt>(GEP->getOperand(1));
+ if (FirstIdx && FirstIdx->isZero()) {
+ // Simple array access: extract dimensions from the underlying array
+ // type
+ Value *Source = GEP->getPointerOperand()->stripPointerCasts();
+ return extractArrayInfoFromAllocaOrGlobal(SE, Source, Sizes,
+ ElementSize);
+ }
+ }
+ // Complex GEPs like (&array[offset])[index] represent pointer arithmetic,
+ // not simple array indexing. These should be handled by parametric
+ // delinearization to preserve the linearized byte-offset semantics rather
+ // than treating them as multidimensional array accesses.
+ return false;
+ }
+
+ // Check if BasePtr is from an alloca instruction.
+ Type *ElementType = nullptr;
+ if (auto *AI = dyn_cast<AllocaInst>(BasePtr)) {
+ ElementType = AI->getAllocatedType();
+ LLVM_DEBUG(dbgs() << "Found alloca with type: " << *ElementType << "\n");
+ } else if (auto *GV = dyn_cast<GlobalVariable>(BasePtr)) {
+ ElementType = GV->getValueType();
+ LLVM_DEBUG(dbgs() << "Found global variable with type: " << *ElementType
+ << "\n");
+ } else {
+ LLVM_DEBUG(dbgs() << "No alloca or global found for base pointer\n");
+ return false;
+ }
+
+ // Extract dimensions from nested array types.
+ Type *I64Ty = Type::getInt64Ty(SE.getContext());
+
+ while (auto *ArrayTy = dyn_cast<ArrayType>(ElementType)) {
+ uint64_t Size = ArrayTy->getNumElements();
+ const SCEV *SizeSCEV = SE.getConstant(I64Ty, Size);
+ Sizes.push_back(SizeSCEV);
+ ElementType = ArrayTy->getElementType();
+ LLVM_DEBUG(dbgs() << " Found array dimension: " << Size << "\n");
+ }
+
+ if (Sizes.empty()) {
+ LLVM_DEBUG(dbgs() << "No array dimensions found in type\n");
+ return false;
+ }
+
+ // Add element size as the last element for computeAccessFunctions algorithm.
+ Sizes.push_back(ElementSize);
+
+ LLVM_DEBUG({
+ dbgs() << "Extracted array info from alloca/global for base pointer "
+ << *BasePtr << "\n";
+ dbgs() << "Dimensions: ";
+ for (const SCEV *Size : Sizes)
+ dbgs() << *Size << " ";
+ dbgs() << "\n";
+ });
+
+ return true;
+}
+
+bool llvm::delinearizeUsingArrayInfo(ScalarEvolution &SE, const SCEV *AccessFn,
+ SmallVectorImpl<const SCEV *> &Subscripts,
+ SmallVectorImpl<const SCEV *> &Sizes,
+ const SCEV *ElementSize) {
+ // Clear output vectors.
+ Subscripts.clear();
+ Sizes.clear();
+
+ const SCEVUnknown *BasePointer =
+ dyn_cast<SCEVUnknown>(SE.getPointerBase(AccessFn));
+ if (!BasePointer) {
+ LLVM_DEBUG(dbgs() << "no BasePointer for AccessFn: " << *AccessFn << "\n");
+ return false;
+ }
+
+ Value *BasePtr = BasePointer->getValue();
+
+ // Extract array dimensions from alloca or global declarations.
+ if (!extractArrayInfoFromAllocaOrGlobal(SE, BasePtr, Sizes, ElementSize))
+ return false;
+
+ // Get the full SCEV expression and subtract the base pointer to get
+ // offset-only expression.
+ const SCEV *Expr = SE.getMinusSCEV(AccessFn, BasePointer);
+
+ computeAccessFunctions(SE, Expr, Subscripts, Sizes, ElementSize);
+ if (Sizes.empty() || Subscripts.empty())
+ return false;
+
+ // Validate dimension consistency: subscripts should match array dimensions
+ // (Sizes includes element size as last element, so array dimensions =
+ // Sizes.size() - 1)
+ unsigned ArrayDims = Sizes.size() - 1;
+ if (Subscripts.size() != ArrayDims) {
+ LLVM_DEBUG(
+ dbgs() << "delinearizeUsingArrayInfo: Dimension mismatch - "
+ << Subscripts.size() << " subscripts for " << ArrayDims
+ << " array dimensions. Falling back to parametric method.\n");
+ return false;
+ }
+
+ return true;
+}
+
/// Splits the SCEV into two vectors of SCEVs representing the subscripts and
/// sizes of an array access. Returns the remainder of the delinearization that
/// is the offset start of the array. The SCEV->delinearize algorithm computes
@@ -454,36 +621,35 @@ void llvm::delinearize(ScalarEvolution &SE, const SCEV *Expr,
SmallVectorImpl<const SCEV *> &Subscripts,
SmallVectorImpl<const SCEV *> &Sizes,
const SCEV *ElementSize) {
- // First step: collect parametric terms.
+ // Clear output vectors.
+ Subscripts.clear();
+ Sizes.clear();
+
+ // Try array_info extraction.
+ if (delinearizeUsingArrayInfo(SE, Expr, Subscripts, Sizes, ElementSize))
+ return;
+ LLVM_DEBUG(dbgs() << "delinearize falling back to parametric method\n");
+
+ // Fall back to parametric delinearization.
+ const SCEVUnknown *BasePointer =
+ dyn_cast<SCEVUnknown>(SE.getPointerBase(Expr));
+ if (BasePointer)
+ Expr = SE.getMinusSCEV(Expr, BasePointer);
+
SmallVector<const SCEV *, 4> Terms;
collectParametricTerms(SE, Expr, Terms);
if (Terms.empty())
return;
- // Second step: find subscript sizes.
+ // Find subscript sizes.
findArrayDimensions(SE, Terms, Sizes, ElementSize);
if (Sizes.empty())
return;
- // Third step: compute the access functions for each subscript.
- computeAccessFunctions(SE, Expr, Subscripts, Sizes);
-
- if (Subscripts.empty())
- return;
-
- LLVM_DEBUG({
- dbgs() << "succeeded to delinearize " << *Expr << "\n";
- dbgs() << "ArrayDecl[UnknownSize]";
- for (const SCEV *S : Sizes)
- dbgs() << "[" << *S << "]";
-
- dbgs() << "\nArrayRef";
- for (const SCEV *S : Subscripts)
- dbgs() << "[" << *S << "]";
- dbgs() << "\n";
- });
+ // Compute the access functions for each subscript.
+ computeAccessFunctions(SE, Expr, Subscripts, Sizes, ElementSize);
}
static std::optional<APInt> tryIntoAPInt(const SCEV *S) {
@@ -492,6 +658,21 @@ static std::optional<APInt> tryIntoAPInt(const SCEV *S) {
return std::nullopt;
}
+/// Convert cached SCEV sizes to int sizes for compatibility.
+/// TODO: Remove this after we remove GEP delinearization.
+static void convertSCEVSizesToIntSizes(ArrayRef<const SCEV *> SCEVSizes,
+ SmallVectorImpl<int> &Sizes) {
+ for (const SCEV *S : SCEVSizes) {
+ if (auto *Const = dyn_cast<SCEVConstant>(S)) {
+ const APInt &APVal = Const->getAPInt();
+ if (APVal.isSignedIntN(32)) {
+ int intValue = APVal.getSExtValue();
+ Sizes.push_back(intValue);
+ }
+ }
+ }
+}
+
/// Collects the absolute values of constant steps for all induction variables.
/// Returns true if we can prove that all step recurrences are constants and \p
/// Expr is divisible by \p ElementSize. Each step recurrence is stored in \p
@@ -646,6 +827,9 @@ bool llvm::delinearizeFixedSizeArray(ScalarEvolution &SE, const SCEV *Expr,
SmallVectorImpl<const SCEV *> &Subscripts,
SmallVectorImpl<const SCEV *> &Sizes,
const SCEV *ElementSize) {
+ // Clear output vectors.
+ Subscripts.clear();
+ Sizes.clear();
// First step: find the fixed array size.
SmallVector<uint64_t, 4> ConstSizes;
@@ -659,7 +843,7 @@ bool llvm::delinearizeFixedSizeArray(ScalarEvolution &SE, const SCEV *Expr,
Sizes.push_back(SE.getConstant(Expr->getType(), Size));
// Second step: compute the access functions for each subscript.
- computeAccessFunctions(SE, Expr, Subscripts, Sizes);
+ computeAccessFunctions(SE, Expr, Subscripts, Sizes, ElementSize);
return !Subscripts.empty();
}
@@ -671,6 +855,7 @@ bool llvm::getIndexExpressionsFromGEP(ScalarEvolution &SE,
assert(Subscripts.empty() && Sizes.empty() &&
"Expected output lists to be empty on entry to this function.");
assert(GEP && "getIndexExpressionsFromGEP called with a null GEP");
+ LLVM_DEBUG(dbgs() << "\nGEP to delinearize: " << *GEP << "\n");
Type *Ty = nullptr;
bool DroppedFirstDim = false;
for (unsigned i = 1; i < GEP->getNumOperands(); i++) {
@@ -683,28 +868,43 @@ bool llvm::getIndexExpressionsFromGEP(ScalarEvolution &SE,
continue;
}
Subscripts.push_back(Expr);
+ LLVM_DEBUG(dbgs() << "Subscripts push_back: " << *Expr << "\n");
continue;
}
auto *ArrayTy = dyn_cast<ArrayType>(Ty);
if (!ArrayTy) {
+ LLVM_DEBUG(dbgs() << "GEP delinearize failed: " << *Ty
+ << " is not an array type.\n");
Subscripts.clear();
Sizes.clear();
return false;
}
Subscripts.push_back(Expr);
+ LLVM_DEBUG(dbgs() << "Subscripts push_back: " << *Expr << "\n");
if (!(DroppedFirstDim && i == 2))
Sizes.push_back(ArrayTy->getNumElements());
Ty = ArrayTy->getElementType();
}
+ LLVM_DEBUG({
+ dbgs() << "Subscripts:\n";
+ for (const SCEV *S : Subscripts)
+ dbgs() << *S << "\n";
+ dbgs() << "\n";
+ });
+
return !Subscripts.empty();
}
bool llvm::tryDelinearizeFixedSizeImpl(
ScalarEvolution *SE, Instruction *Inst, const SCEV *AccessFn,
SmallVectorImpl<const SCEV *> &Subscripts, SmallVectorImpl<int> &Sizes) {
+ // Clear output vectors.
+ Subscripts.clear();
+ Sizes.clear();
+
Value *SrcPtr = getLoadStorePointerOperand(Inst);
// Check the simple case where the array dimensions are fixed size.
@@ -712,7 +912,140 @@ bool llvm::tryDelinearizeFixedSizeImpl(
if (!SrcGEP)
return false;
- getIndexExpressionsFromGEP(*SE, SrcGEP, Subscripts, Sizes);
+ // When flag useGEPToDelinearize is false, delinearize only using array_info.
+ if (!useGEPToDelinearize) {
+ SmallVector<const SCEV *, 4> SCEVSizes;
+ const SCEV *ElementSize = SE->getElementSize(Inst);
+ if (!delinearizeUsingArrayInfo(*SE, AccessFn, Subscripts, SCEVSizes,
+ ElementSize))
+ return false;
+
+ // TODO: Remove the following code. Convert SCEV sizes to int sizes. This
+ // conversion is only needed as long as getIndexExpressionsFromGEP is still
+ // around. Remove this code and change the interface of
+ // tryDelinearizeFixedSizeImpl to take a SmallVectorImpl<const SCEV *>
+ // &Sizes.
+ convertSCEVSizesToIntSizes(SCEVSizes, Sizes);
+ return true;
+ }
+
+ // TODO: Remove all the following code once we are satisfied with array_info.
+ // Run both methods when useGEPToDelinearize is true: validation is enabled.
+
+ // Store results from both methods.
+ SmallVector<const SCEV *, 4> GEPSubscripts, ArrayInfoSubscripts;
+ SmallVector<int, 4> GEPSizes, ArrayInfoSizes;
+
+ // GEP-based delinearization.
+ bool GEPSuccess =
+ getIndexExpressionsFromGEP(*SE, SrcGEP, GEPSubscripts, GEPSizes);
+
+ // Array_info delinearization.
+ SmallVector<const SCEV *, 4> SCEVSizes;
+ const SCEV *ElementSize = SE->getElementSize(Ins...
[truncated]
|
Why not? I find this solution more suited than other approaches because we don't have to change LLVM's IR. There's an alternative solution that requires an RFC to change LLVM IR: see comment on the first patch of the series.
|
|
It would be best to consult an LLVM IR expert. I'm not an expert, but from my perspective, using the type arguments of allocas and/or global variables doesn't seem to significantly different from driving heuristics based on those of GEPs. |
sebpop
force-pushed
the
delin-alloca-global
branch
from
65e9cfc to
62ac07f
Compare
…tions This implements an alternative to extract multi-dimensional array information directly from alloca instructions and global variable declarations instead of relying on parsing the information from GEP instructions. The functionality for GEP delinearization remains in this patch such that we can evaluate regressions of the array_info extraction. Next patch removes the code for GEP delinearization.
sebpop
force-pushed
the
delin-alloca-global
branch
from
62ac07f to
274651b
Compare
I guess Sebastian's discourse message is a reply to this. We are not allowed to look at GEP->Type, but this information has to come from somewhere and the question is where? This information is present at statically declared arrays, and I don't see it disappearing from there because it has to be there, I think. For alloca, @nikic is suggesting that |
Watching the video, it sounds like it's clearly stated that using the type in |
|
To be clear: I don't think this is a right approach. In that presentation, it is explicitly stated that using the type of I've long wondered, why are you so particular about array type information? A delinearization function without relying on type information has already been landed (#145050). Isn't it insufficient? It is true that driving the heuristic using type information (which is actually disallowed) can sometimes work better. However, even with the one added in #145050, I believe the basic cases should be covered. In addition, there is room for improvement to catch more cases. In my humble opinion, improving the delienearization without relying on type information would make things go much more smoothly. Or are there specific cases you want to cover that definitely require type information? (This is purely my personal opinion, and I don't have any data, but) even if there were cases that could not be recovered without using type information, I don’t believe that would amount to a critical problem. Such cases tend to be difficult to analyze for dependencies (or a dependence exists in the first place), and even if the dependence analysis works perfectly, I don’t really think it would result in a situation where loop-interchange could be applied (though I don’t know about any other passes that use DA). To summarize, I personally believe that cases where the presence or absence of type information changes whether the optimization is applied are extremely rare. Moreover, what I find most puzzling is that, while there is such great concern for those (I think) rare cases, at the same time downplaying the numerous potential miscompilations on the grounds that they are "corner cases". In a production-level compiler, miscompilations should be considered far more important than missed optimizations, I believe. |
Yes, it is insufficient. Please see my ask to enable that code in all places where we call delinearize #145050 (comment) |
There was a problem hiding this comment.
I think the type of a global is a valid source of information for heuristics, but:
- Only few actual programs store their data in
.bsssections, so application of this heuristic is quite limited - I think the general direction of "de-typification" is to also see globals as a sequence of bytes of predefined length, there just is no reason (yet) to change the declaration syntax. Long-term, that may change.
Both means we cannot exclusively rely on global types for a source of information, but I think it is OK for an additional souce of information.
|
|
||
| getIndexExpressionsFromGEP(*SE, SrcGEP, Subscripts, Sizes); | ||
| // When flag useGEPToDelinearize is false, delinearize only using array_info. | ||
| if (!useGEPToDelinearize) { |
There was a problem hiding this comment.
Could you make "delinerize-from-GEP" and "delinerize-from-global-type" each boolean enable options (like UseFixedSizeArrayHeuristic). So when "delinerize-from-global-type", it can still fall back to GEP delinearization? This way, we can prepare GEP removal by just switch off that "delinerize-from-GEP" flag. We also do not need to update all tests at once, but can be done incrementally.
| return false; | ||
|
|
||
| getIndexExpressionsFromGEP(*SE, SrcGEP, Subscripts, Sizes); | ||
| // When flag useGEPToDelinearize is false, delinearize only using array_info. |
There was a problem hiding this comment.
array_info probably refers to another patch, it is not currently that appears in LLVM-IR.
I'll answer your question here.
I intended to replace the use of GEP-dependent functions with that later. I just have not gotten around to it yet. Since I figured it would be quicker to just show my thought, I created a draft PR #161822. What I meant by my question was: is this direction not sufficient for your needs? |
|
Thank you for #161822, yes this is a good direction and it is orthogonal to the patch to reads the info from global and alloca types. (By orthogonal I mean: we may recover enough info just from the shape of memory access functions without external help from the front-ends.)
I don't know yet: we will know if |
What do you mean by "all the use-cases in the test suite"? I think it’s impractical to fully reproduce the current GEP-based delinearization behavior without relying on GEP’s type information. While Polly shouldn’t be relevant here, even if it uses GEP-based delinearization as well. |
| } | ||
|
|
||
| // Extract dimensions from nested array types. | ||
| Type *I64Ty = Type::getInt64Ty(SE.getContext()); |
There was a problem hiding this comment.
Better to use DataLayout::getIndexSize to get the size of indices
There was a problem hiding this comment.
Could you please elaborate on how using DataLayout::getIndexSize would work?
| if (APVal.isSignedIntN(32)) { | ||
| int intValue = APVal.getSExtValue(); | ||
| Sizes.push_back(intValue); | ||
| } |
There was a problem hiding this comment.
What happens if it is not signed int32 value?
There was a problem hiding this comment.
Bad things may happen. Seriously: the delinearization shouldn't use an int32 vector to keep array sizes, it should be a vector of SCEV expressions.
The comment above the function says:
/// TODO: Remove this after we remove GEP delinearization.
The vector of int32 is only used in the interface of GEP-delinearize, not in the rest of the delinearization analysis where we use a vector of SCEV exprs.
I could try to do this cleanup before this patch gets merged in.
There was a problem hiding this comment.
Clean-up posted here: #162888
Once that gets committed I will update the current PR and remove the above loop and conversions for Sizes vector from SCEV to int and back to SCEV.
| } | ||
|
|
||
| // Compare subscripts arrays. | ||
| if (GEPSubscripts.size() != ArrayInfoSubscripts.size()) { |
There was a problem hiding this comment.
Is this supposed to be an assertion? It has no actual code, just debug prints.
There was a problem hiding this comment.
This is a diff between two ways of computing the same thing GEP and array_info (whether it is attached to assumes or to alloca/globals or getting the array_info from the current types of alloca/globals.)
I decided to not call abort on differences, and just count the number of WARN messages.
Next patch in the series #156215 removes all the GEP related computations, so the diff gets removed as well.
| @A.global = global [100 x [100 x i32]] zeroinitializer | ||
|
|
||
| ;; for (long int i = 0; i < 50; i++) { | ||
| ;; A[i][i] = i; | ||
| ;; *B++ = A[i + 10][i + 9]; | ||
|
|
||
| define void @couple0(ptr %A, ptr %B, i32 %n) nounwind uwtable ssp { | ||
| define void @couple0(ptr %B, i32 %n) nounwind uwtable ssp { |
There was a problem hiding this comment.
This (same applies elsewhere) seems to change the meaning of the test. I think we should avoid such changes.
There was a problem hiding this comment.
Please elaborate over "seems to change the meaning of the test".
How does this changes what the test checks for?
I believe the test remains the exact same, there are no changes to the CHECK: statements.
There was a problem hiding this comment.
You might want to consider the purpose of regression tests. I believe one of the primary purposes of regression tests is to guarantee that nothing unexpected happens. From this standpoint, it doesn’t make sense to modify the input to keep the output unchanged.
If this patch changes the output for this input, it would be considered a regression, and we need to check whether the change is reasonable or not.
There was a problem hiding this comment.
I agree that patches should have minimal or no changes at all to the existing tests.
However this is a patch out of a patch series. Next patch #156215 in the series removes support for GEP-delinearize analysis. So in this context, the current patch modifies the testsuite such that there are no differences (WARN messages) between GEP-delinearize and the array_info-delinearize. With these minimal changes we can remove GEP-delinearize and keep all the tests.
Without changes to the testsuite, all those tests would regress by removing GEP-delinearize, and the tests would stop testing different other aspects of the compiler that they have been added to test (i.e., polly scheduling, DA tests, etc.)
There was a problem hiding this comment.
If I remove all changes to Coupled.ll and run lit on it, the test passes:
./bin/llvm-lit -sv ../llvm/test/Analysis/DependenceAnalysis/Coupled.ll
However if I add -debug to the test, it will show several mismatches:
WARNING: array_info failed and GEP analysis succeeded.
Instruction: store i32 1, ptr %arrayidx.us, align 4
Using GEP analysis results despite array_info failure
Now if we apply the next patch in the series, this warn message will be removed and the delinearization fails, and the test does not test for the same thing (DA results) anymore.
There was a problem hiding this comment.
So you're saying the following, right?
- Some test cases were added to test specific features (e.g., strongSIV, exactSIV), and they depend on the results of delinearization. If delinearization fails, these tests no longer make sense.
- To keep these tests meaningful, we need to ensure that delinearization continues to work.
Assuming that's correct, I'd suggest the following approach:
- Could you separate these test changes into another PR? I believe we can make this patch a dedicated one that focuses solely on the new feature (i.e., using type information from global type declarations as hints).
- I think such large changes to regression tests are not needed, because
- Some of them will be removed at all in [DA] remove constraint propagation and coupled/separable subscripts #160924.
- I've not checked the details yet, but [DA] Replace delinearization for fixed size array #161822 seems to cover a fair number of existing tests.
There was a problem hiding this comment.
Yes, correct on all points.
I will split the patch into two.
There was a problem hiding this comment.
I split the llvm tests in 117c20c.
The changes to polly/test are too hard to split.
| %sub = sub nsw i64 18, %i.02 | ||
| %mul = mul nsw i64 %i.02, 3 | ||
| %sub1 = add nsw i64 %mul, -18 | ||
| %sub1 = add nsw i64 %mul, 18 |
There was a problem hiding this comment.
If this patch degrades test results, I believe that should be considered a regression. In such cases, modifying the input IR doesn't seem like a proper approach to me. Notably, DA should allow the subscript being negative value for the outermost dimension.
There was a problem hiding this comment.
This negative subscript would be out of bounds, so the original testcase is broken from a c/c++ or fortran point of view. See my comment above on the C code this LLVM IR comes from.
There was a problem hiding this comment.
I think the original test can be interpreted as follows.
void couple11(int A[][100], int B[]) {
for (long int i = 0; i <= 15; i++) {
A[3*i - 18][18 - i] = i;
B[i] = A[i][i];
}
}IIUC, the following C code doesn't cause out-of-bounds.
int Mem[100][100] = {};
int (*A)[100] = Mem + 50;
int B[16];
couple11(A, B);In addition, this is LLVM, not C/C++/Fortran.
There was a problem hiding this comment.
That proves my point that adding more specifics to the testsuite from the C/C++/Fortran front-end helps disambiguate what the LLVM IR testcase means.
In this particular case, we should add to this global decl:
@Mem = global [100 x [100 x i32]] zeroinitializer
several more statements to show how we got to that negative subscript. That is this computation translated to LLVM IR:
int (*A)[100] = Mem + 50;
I believe we need a bit more offset here than Mem + 50: at least Mem + 18 * 100 or so.
There was a problem hiding this comment.
My argument is: please do not rewrite the input (see also #156342 (comment)). If you want to test the feature that using type information of global decls as heuristic guide, you should add a new one instead of changing the existing one.
int (*A)[100] = Mem + 50;I believe we need a bit more offset here than
Mem + 50: at leastMem + 18 * 100or so.
IIUC, Mem + 50 points to the same location as &Mem[50][0].
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
| << *BasePtr << "\n"); | ||
|
|
||
| // Distinguish between simple array accesses and complex pointer arithmetic. | ||
| // Only apply array_info extraction to direct array accesses to avoid |
There was a problem hiding this comment.
The term array_info doesn't seem to be defined anywhere.
There was a problem hiding this comment.
This is a remnant of the previous attempt sebpop@51e0f4f that was to attach an array_info bundle to assumes. We may want to revive that patch and instead of attaching the array_info bundle to an assume, we will attach the array_info bundles to allocas and to globals. Right now array_info is the same as 'type' of alloca and globals.
Co-authored-by: Ryotaro Kasuga <[email protected]>
7 participants
This implements an alternative to extract multi-dimensional array information
directly from alloca instructions and global variable declarations instead of
relying on parsing the information from GEP instructions.
The functionality for GEP delinearization remains in this patch such that we can
evaluate regressions of the array_info extraction.
Next patch removes the code for GEP delinearization.