-
Notifications
You must be signed in to change notification settings - Fork 14.4k
[Delinearization] Modernize loops (NFC) #146151
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@llvm/pr-subscribers-llvm-analysis Author: Ramkumar Ramachandra (artagnon) ChangesFull diff: https://github.com/llvm/llvm-project/pull/146151.diff 1 Files Affected:
diff --git a/llvm/lib/Analysis/Delinearization.cpp b/llvm/lib/Analysis/Delinearization.cpp
index 329bd35530c72..a35d411d66300 100644
--- a/llvm/lib/Analysis/Delinearization.cpp
+++ b/llvm/lib/Analysis/Delinearization.cpp
@@ -349,14 +349,13 @@ void llvm::computeAccessFunctions(ScalarEvolution &SE, const SCEV *Expr,
return;
const SCEV *Res = Expr;
- int Last = Sizes.size() - 1;
- for (int i = Last; i >= 0; i--) {
+ for (const auto &[Idx, Sz] : enumerate(reverse(Sizes))) {
const SCEV *Q, *R;
- SCEVDivision::divide(SE, Res, Sizes[i], &Q, &R);
+ SCEVDivision::divide(SE, Res, Sz, &Q, &R);
LLVM_DEBUG({
dbgs() << "Res: " << *Res << "\n";
- dbgs() << "Sizes[i]: " << *Sizes[i] << "\n";
+ dbgs() << "Sizes[i]: " << *Sz << "\n";
dbgs() << "Res divided by Sizes[i]:\n";
dbgs() << "Quotient: " << *Q << "\n";
dbgs() << "Remainder: " << *R << "\n";
@@ -364,9 +363,8 @@ void llvm::computeAccessFunctions(ScalarEvolution &SE, const SCEV *Expr,
Res = Q;
- // Do not record the last subscript corresponding to the size of elements in
- // the array.
- if (i == Last) {
+ // Do not record the last subscript.
+ if (Idx == 0) {
// Bail out if the byte offset is non-zero.
if (!R->isZero()) {
@@ -489,9 +487,9 @@ bool llvm::getIndexExpressionsFromGEP(ScalarEvolution &SE,
assert(GEP && "getIndexExpressionsFromGEP called with a null GEP");
Type *Ty = nullptr;
bool DroppedFirstDim = false;
- for (unsigned i = 1; i < GEP->getNumOperands(); i++) {
- const SCEV *Expr = SE.getSCEV(GEP->getOperand(i));
- if (i == 1) {
+ for (const auto [Idx, Op] : drop_begin(enumerate(GEP->operands()))) {
+ const SCEV *Expr = SE.getSCEV(Op);
+ if (Idx == 1) {
Ty = GEP->getSourceElementType();
if (auto *Const = dyn_cast<SCEVConstant>(Expr))
if (Const->getValue()->isZero()) {
@@ -510,7 +508,7 @@ bool llvm::getIndexExpressionsFromGEP(ScalarEvolution &SE,
}
Subscripts.push_back(Expr);
- if (!(DroppedFirstDim && i == 2))
+ if (!(DroppedFirstDim && Idx == 2))
Sizes.push_back(ArrayTy->getNumElements());
Ty = ArrayTy->getElementType();
@@ -596,13 +594,13 @@ void printDelinearization(raw_ostream &O, Function *F, LoopInfo *LI,
O << "Base offset: " << *BasePointer << "\n";
O << "ArrayDecl[UnknownSize]";
int Size = Subscripts.size();
- for (int i = 0; i < Size - 1; i++)
- O << "[" << *Sizes[i] << "]";
+ for (const SCEV *Sz : drop_end(Sizes))
+ O << "[" << *Sz << "]";
O << " with elements of " << *Sizes[Size - 1] << " bytes.\n";
O << "ArrayRef";
- for (int i = 0; i < Size; i++)
- O << "[" << *Subscripts[i] << "]";
+ for (const SCEV *Sub : Subscripts)
+ O << "[" << *Sub << "]";
O << "\n";
}
}
|
// Do not record the last subscript corresponding to the size of elements in | ||
// the array. | ||
if (i == Last) { | ||
// Do not record the last subscript. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this comment updated?
// the array. | ||
if (i == Last) { | ||
// Do not record the last subscript. | ||
if (Idx == 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I personally feel the previous one is more readable...
|
||
LLVM_DEBUG({ | ||
dbgs() << "Res: " << *Res << "\n"; | ||
dbgs() << "Sizes[i]: " << *Sizes[i] << "\n"; | ||
dbgs() << "Sizes[i]: " << *Sz << "\n"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you keep the variable name as it is? This is no longer Sizes[i]
.
for (unsigned i = 1; i < GEP->getNumOperands(); i++) { | ||
const SCEV *Expr = SE.getSCEV(GEP->getOperand(i)); | ||
if (i == 1) { | ||
for (const auto [Idx, Op] : drop_begin(enumerate(GEP->operands()))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for (const auto [Idx, Op] : drop_begin(enumerate(GEP->operands()))) { | |
for (const auto &[Idx, Op] : drop_begin(enumerate(GEP->operands()))) { |
In addition, this function should be removed because it uses the GEP source element type for its optimization heuristics. IMHO, such a function should not be modified unless it causes a serious issue.
No description provided.