[PATCH] [SLSR] handle candidate form (B + i * S)
Bjarke Hammersholt Roune
broune at google.com
Tue Apr 14 13:08:24 PDT 2015
I added some comments not directly related to the patch, since you're the author of the whole pass, but feel free to ignore those parts for this patch.
Would it make sense to avoid transformations like this one?
X = B + 8 * S
Y = B + S
to
X = B + 8 * S
Y = X - 7 * S
================
Comment at: lib/Transforms/Scalar/StraightLineStrengthReduce.cpp:29
@@ -28,1 +28,3 @@
+// S1: X = B + i * S
+// S2: Y = B + i' * S => X + (i' - i) * S
//
----------------
Might be good to explain here why this is an improvement and where the strength is reduced. The rewritten expression still has the form B + i * S, just with a different B and i. Right? Perhaps we're hoping that (i' - i) * S will be easier to optimize, e.g. if it appears more than once or if i' - i is now -1, 0, 1 or a power of 2?
================
Comment at: lib/Transforms/Scalar/StraightLineStrengthReduce.cpp:35
@@ -31,7 +34,3 @@
// S1: X = &B[i * S]
-// S2: Y = &B[i' * S]
-//
-// and S1 dominates S2, we call S1 a basis of S2, and can replace S2 with
-//
-// Y = X + (i' - i) * S
+// S2: Y = &B[i' * S] => &X[(i' - i) * S]
//
----------------
Same comment as above.
================
Comment at: lib/Transforms/Scalar/StraightLineStrengthReduce.cpp:37
@@ -37,3 +36,3 @@
//
-// or
+// Note: (i' - i) * S is folded to the extend possible.
//
----------------
extend -> extent
================
Comment at: lib/Transforms/Scalar/StraightLineStrengthReduce.cpp:41
@@ -44,1 +40,3 @@
+// multiple bases, we choose to rewrite S2 with respect to the closest basis or
+// the "immediate" basis.
//
----------------
I think this is a definition of what an immediate basis is, though it reads to me as a statement that we either use the closest basis or the immediate basis, which suggests that they are not necessarily the same. Perhaps: with respect to the immediate basis, which is the basis that is the closest ancestor in the dominator tree." Is that what "closest" means here?
================
Comment at: lib/Transforms/Scalar/StraightLineStrengthReduce.cpp:49
@@ -52,3 +48,3 @@
// sensitive to ILP may want to disable it. Having SLSR to consider ILP is
// left as future work.
#include <vector>
----------------
Would it make sense to add a TODO for the case where i - i' is constant but i and i' are not?
================
Comment at: lib/Transforms/Scalar/StraightLineStrengthReduce.cpp:74
@@ -77,3 +73,3 @@
// or
// Base[..][Index * Stride][..]
struct Candidate : public ilist_node<Candidate> {
----------------
Should there be another case added above for Add, or perhaps just refer to the comments on the entries in Kind instead of duplicating it here?
================
Comment at: lib/Transforms/Scalar/StraightLineStrengthReduce.cpp:92
@@ -94,3 +91,3 @@
const SCEV *Base;
// Note that Index and Stride of a GEP candidate may not have the same
// integer type. In that case, during rewriting, Stride will be
----------------
This is ambiguous as "may not" can mean "must not" or "might not". I'd do "may not" -> "might not" or rewrite as "does not necessarily have". (I know you didn't change this in this patch, but you're the author of the pass, right?)
================
Comment at: lib/Transforms/Scalar/StraightLineStrengthReduce.cpp:99
@@ -101,3 +98,3 @@
// candidate with respect to its immediate basis. Note that one instruction
// can corresponds to multiple candidates depending on how you associate the
// expression. For instance,
----------------
can corresponds -> can correspond
================
Comment at: lib/Transforms/Scalar/StraightLineStrengthReduce.cpp:146
@@ -148,1 +145,3 @@
void allocateCandidateAndFindBasis(Instruction *I);
+ // Allocate candidates and find bases for Add instructions.
+ void allocateCandidateAndFindBasisForAdd(Instruction *I);
----------------
The function name says singular candidate and the comment says plural candidates. So one of them is not right.
================
Comment at: lib/Transforms/Scalar/StraightLineStrengthReduce.cpp:193
@@ -189,3 +192,3 @@
// after all rewriting finishes.
DenseSet<Instruction *> UnlinkedInstructions;
};
----------------
We also keep track of unlinked instructions by their parent being null, and therefore never insert the same element twice nor query whether an element is in UnlinkedInstructions. So might this better be a lower-overhead type like a vector?
================
Comment at: lib/Transforms/Scalar/StraightLineStrengthReduce.cpp:265
@@ -255,2 +264,3 @@
+
// TODO: We currently implement an algorithm whose time complexity is linear to
// the number of existing candidates. However, a better algorithm exists. We
----------------
to -> in
================
Comment at: lib/Transforms/Scalar/StraightLineStrengthReduce.cpp:283
@@ -269,1 +282,3 @@
+ // Similarly, bail out if (B + Idx * S) is free.
+ if (isAddFoldable(B, Idx, S, TTI))
return;
----------------
Maybe add: in case this expression is later used as an address in a way that is foldable as an addressing mode.
Or am I misunderstanding this? The significant difference seems to me to be that above, we know it's an address and thus will be folded if possible as it's included directly in a GEP and here this expression may or may not feed directly into a GEP later on.
================
Comment at: lib/Transforms/Scalar/StraightLineStrengthReduce.cpp:290
@@ -274,3 +289,3 @@
unsigned NumIterations = 0;
// Limit the scan radius to avoid running forever.
static const unsigned MaxNumIterations = 50;
----------------
running forever -> taking too much time. It would terminate.
================
Comment at: lib/Transforms/Scalar/StraightLineStrengthReduce.cpp:301
@@ -285,3 +300,3 @@
// Regardless of whether we find a basis for C, we need to push C to the
// candidate list.
Candidates.push_back(C);
----------------
Could add: so that it can be the basis of other candidates.
================
Comment at: lib/Transforms/Scalar/StraightLineStrengthReduce.cpp:374
@@ -324,3 +373,3 @@
Value *LHS = I->getOperand(0), *RHS = I->getOperand(1);
allocateCandidateAndFindBasisForMul(LHS, RHS, I);
----------------
Would it make sense to assert that I has exactly 2 operands?
================
Comment at: lib/Transforms/Scalar/StraightLineStrengthReduce.cpp:399
@@ -349,3 +398,3 @@
GetElementPtrInst *GEP) {
// At least, ArrayIdx = ArrayIdx *s 1.
allocateCandidateAndFindBasisForGEP(
----------------
is *s a typo?
================
Comment at: lib/Transforms/Scalar/StraightLineStrengthReduce.cpp:508
@@ +507,3 @@
+ Value *ExtendedStride = Builder.CreateSExtOrTrunc(C.Stride, DeltaType);
+ if (IndexOffset.isPowerOf2()) {
+ // If (i' - i) is a power of 2, Bump = sext/trunc(S) << log(i' - i).
----------------
would it make sense to catch the negative of a power of 2?
================
Comment at: lib/Transforms/Scalar/StraightLineStrengthReduce.cpp:520
@@ -460,3 +519,3 @@
assert(C.CandidateKind == Basis.CandidateKind && C.Base == Basis.Base &&
C.Stride == Basis.Stride);
----------------
Would it make sense to assert that Basis.Ins->getParent() != nullptr ?
http://reviews.llvm.org/D8983
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
More information about the llvm-commits
mailing list