[LAA] Rework and rename stripGetElementPtr #125315
Conversation
llvmbot
added
the
llvm:analysis
|
@llvm/pr-subscribers-llvm-analysis Author: Ramkumar Ramachandra (artagnon) ChangesThe stripGetElementPtr function is mysteriously named, and calls into another mysterious getGEPInductionOperand which does something complicated with GEP indices. The real purpose of the badly-named stripGetElementPtr function is to get a loop-variant GEP index, if there is one. The getGEPInductionOperand is totally redundant, as stripping off zeros from the end of GEP indices has no effect on computing the loop-variant GEP index, as constant zeros are always loop-invariant. Moreover, the GEP induction operand is simply the first non-zero index from the end, which stripGetElementPtr returns when it finds that any of the GEP indices are loop-variant: this is a completely unrelated value to the GEP index that is loop-variant. The implicit assumption here is that there is only ever one loop-variant index, and it is the first non-zero one from the end. The logic is unnecessarily complicated for what stripGetElementPtr wants to achieve, and the header comments are confusing as well. Strip getGEPInductionOperand, rework and rename stripGetElementPtr. Full diff: https://github.com/llvm/llvm-project/pull/125315.diff 1 Files Affected:
diff --git a/llvm/lib/Analysis/LoopAccessAnalysis.cpp b/llvm/lib/Analysis/LoopAccessAnalysis.cpp
index ac8a35fbc54fb1..e7695ed64ba4f1 100644
--- a/llvm/lib/Analysis/LoopAccessAnalysis.cpp
+++ b/llvm/lib/Analysis/LoopAccessAnalysis.cpp
@@ -42,13 +42,12 @@
#include "llvm/IR/DiagnosticInfo.h"
#include "llvm/IR/Dominators.h"
#include "llvm/IR/Function.h"
-#include "llvm/IR/GetElementPtrTypeIterator.h"
#include "llvm/IR/InstrTypes.h"
#include "llvm/IR/Instruction.h"
#include "llvm/IR/Instructions.h"
+#include "llvm/IR/IntrinsicInst.h"
#include "llvm/IR/Operator.h"
#include "llvm/IR/PassManager.h"
-#include "llvm/IR/PatternMatch.h"
#include "llvm/IR/Type.h"
#include "llvm/IR/Value.h"
#include "llvm/IR/ValueHandle.h"
@@ -66,7 +65,6 @@
#include <vector>
using namespace llvm;
-using namespace llvm::PatternMatch;
#define DEBUG_TYPE "loop-accesses"
@@ -2809,50 +2807,17 @@ bool LoopAccessInfo::isInvariant(Value *V) const {
return SE->isLoopInvariant(S, TheLoop);
}
-/// Find the operand of the GEP that should be checked for consecutive
-/// stores. This ignores trailing indices that have no effect on the final
-/// pointer.
-static unsigned getGEPInductionOperand(const GetElementPtrInst *Gep) {
- const DataLayout &DL = Gep->getDataLayout();
- unsigned LastOperand = Gep->getNumOperands() - 1;
- TypeSize GEPAllocSize = DL.getTypeAllocSize(Gep->getResultElementType());
-
- // Walk backwards and try to peel off zeros.
- while (LastOperand > 1 && match(Gep->getOperand(LastOperand), m_Zero())) {
- // Find the type we're currently indexing into.
- gep_type_iterator GEPTI = gep_type_begin(Gep);
- std::advance(GEPTI, LastOperand - 2);
-
- // If it's a type with the same allocation size as the result of the GEP we
- // can peel off the zero index.
- TypeSize ElemSize = GEPTI.isStruct()
- ? DL.getTypeAllocSize(GEPTI.getIndexedType())
- : GEPTI.getSequentialElementStride(DL);
- if (ElemSize != GEPAllocSize)
- break;
- --LastOperand;
- }
-
- return LastOperand;
-}
-
-/// If the argument is a GEP, then returns the operand identified by
-/// getGEPInductionOperand. However, if there is some other non-loop-invariant
-/// operand, it returns that instead.
-static Value *stripGetElementPtr(Value *Ptr, ScalarEvolution *SE, Loop *Lp) {
+/// If \p Ptr is a GEP, which has a loop-variant index, return that index.
+/// Otherwise, return \p Ptr.
+static Value *getLoopVariantGEPIdx(Value *Ptr, ScalarEvolution *SE, Loop *Lp) {
auto *GEP = dyn_cast<GetElementPtrInst>(Ptr);
if (!GEP)
return Ptr;
- unsigned InductionOperand = getGEPInductionOperand(GEP);
-
- // Check that all of the gep indices are uniform except for our induction
- // operand.
- for (unsigned I = 0, E = GEP->getNumOperands(); I != E; ++I)
- if (I != InductionOperand &&
- !SE->isLoopInvariant(SE->getSCEV(GEP->getOperand(I)), Lp))
- return Ptr;
- return GEP->getOperand(InductionOperand);
+ auto *It = find_if(GEP->operands(), [&](const Use &U) {
+ return !SE->isLoopInvariant(SE->getSCEV(U), Lp);
+ });
+ return It == GEP->op_end() ? Ptr : It->get();
}
/// Get the stride of a pointer access in a loop. Looks for symbolic
@@ -2867,7 +2832,7 @@ static const SCEV *getStrideFromPointer(Value *Ptr, ScalarEvolution *SE, Loop *L
// pointer, otherwise, we are analyzing the index.
Value *OrigPtr = Ptr;
- Ptr = stripGetElementPtr(Ptr, SE, Lp);
+ Ptr = getLoopVariantGEPIdx(Ptr, SE, Lp);
const SCEV *V = SE->getSCEV(Ptr);
if (Ptr != OrigPtr)
|
|
Gentle ping. |
artagnon
force-pushed
the
laa-strip-gep
branch
from
0480a73 to
a9a4e1f
Compare
|
The title/description sound like this would just be updating comments ("clarify"), but the diff is code changes. If this is a refactoring/cleanup, please add NFC and update the title/description. It should start with what it is doing (the "Strip getGEPInductionOperand, rework and rename stripGetElementPtr" part), then elaborate the motivation for it. |
artagnon
changed the title
I've updated the title, and the patch is not exactly a NFC, but I'm not sure it would be wise to add a test as @nikic is working on canonicalizing GEPs with variable offsets. |
There was a problem hiding this comment.
The title/description sound like this would just be updating comments ("clarify"), but the diff is code changes. If this is a refactoring/cleanup, please add NFC and update the title/description. It should start with what it is doing (the "Strip getGEPInductionOperand, rework and rename stripGetElementPtr" part), then elaborate the motivation for it.
I've updated the title, and the patch is not exactly a NFC, but I'm not sure it would be wise to add a test as @nikic is working on canonicalizing GEPs with variable offsets.
If it is possible to add a test to cover the new behavior it would be good to clarify the functional impact and make sure we don't accidentally regress it. I don't think the test would hinder any other efforts?
The stripGetElementPtr function is mysteriously named, and calls into another mysterious getGEPInductionOperand which does something complicated with GEP indices. The real purpose of the badly-named stripGetElementPtr function is to get a loop-variant GEP index, if there is one. The getGEPInductionOperand is totally redundant, as stripping off zeros from the end of GEP indices has no effect on computing the loop-variant GEP index, as constant zeros are always loop-invariant. Moreover, the GEP induction operand is simply the first non-zero index from the end, which stripGetElementPtr returns when it finds that any of the GEP indices are loop-variant: this is a completely unrelated value to the GEP index that is loop-variant. The implicit assumption here is that there is only ever one loop-variant index, and it is the first non-zero one from the end. The logic is unnecessarily complicated for what stripGetElementPtr wants to achieve, and the header comments are confusing as well. Strip getGEPInductionOperand, rework and rename stripGetElementPtr.
artagnon
force-pushed
the
laa-strip-gep
branch
from
a9a4e1f to
1752ccd
Compare
Done, thanks! |
Meinersbur
changed the title
The stripGetElementPtr function is mysteriously named, and calls into another mysterious getGEPInductionOperand which does something complicated with GEP indices. The real purpose of the badly-named stripGetElementPtr function is to get a loop-variant GEP index, if there is one. The getGEPInductionOperand is totally redundant, as stripping off zeros from the end of GEP indices has no effect on computing the loop-variant GEP index, as constant zeros are always loop-invariant. Moreover, the GEP induction operand is simply the first non-zero index from the end, which stripGetElementPtr returns when it finds that any of the GEP indices are loop-variant: this is a completely unrelated value to the GEP index that is loop-variant. The implicit assumption here is that there is only ever one loop-variant index, and it is the first non-zero one from the end. The logic is unnecessarily complicated for what stripGetElementPtr wants to achieve, and the header comments are confusing as well. Strip getGEPInductionOperand, rework and rename stripGetElementPtr.
The stripGetElementPtr function is mysteriously named, and calls into another mysterious getGEPInductionOperand which does something complicated with GEP indices. The real purpose of the badly-named stripGetElementPtr function is to get a loop-variant GEP index, if there is one. The getGEPInductionOperand is totally redundant, as stripping off zeros from the end of GEP indices has no effect on computing the loop-variant GEP index, as constant zeros are always loop-invariant. Moreover, the GEP induction operand is simply the first non-zero index from the end, which stripGetElementPtr returns when it finds that any of the GEP indices are loop-variant: this is a completely unrelated value to the GEP index that is loop-variant. The implicit assumption here is that there is only ever one loop-variant index, and it is the first non-zero one from the end.
The logic is unnecessarily complicated for what stripGetElementPtr wants to achieve, and the header comments are confusing as well. Strip getGEPInductionOperand, rework and rename stripGetElementPtr.