[PATCH] [SLSR] handle candidate form (B + i * S)
Jingyue Wu
jingyue at google.com
Tue Apr 14 22:03:19 PDT 2015
That's a very good point. I think `isSimplestForm` in the updated patch handles that case. I also added a test. PTAL
Thanks for the detailed review! These comments improved the patch a lot.
================
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
//
----------------
broune wrote:
> 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?
Done.
================
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]
//
----------------
broune wrote:
> Same comment as above.
Done.
================
Comment at: lib/Transforms/Scalar/StraightLineStrengthReduce.cpp:37
@@ -37,3 +36,3 @@
//
-// or
+// Note: (i' - i) * S is folded to the extend possible.
//
----------------
broune wrote:
> extend -> extent
Done.
================
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.
//
----------------
broune wrote:
> 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?
Done.
================
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>
----------------
broune wrote:
> Would it make sense to add a TODO for the case where i - i' is constant but i and i' are not?
Done. I don't think it's a common case, unless you encountered that already? Anyway, it doesn't hurt adding a TODO.
================
Comment at: lib/Transforms/Scalar/StraightLineStrengthReduce.cpp:74
@@ -77,3 +73,3 @@
// or
// Base[..][Index * Stride][..]
struct Candidate : public ilist_node<Candidate> {
----------------
broune wrote:
> 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?
Done.
================
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
----------------
broune wrote:
> 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?)
Done.
================
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,
----------------
broune wrote:
> can corresponds -> can correspond
Done.
================
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);
----------------
broune wrote:
> The function name says singular candidate and the comment says plural candidates. So one of them is not right.
Done.
================
Comment at: lib/Transforms/Scalar/StraightLineStrengthReduce.cpp:193
@@ -189,3 +192,3 @@
// after all rewriting finishes.
DenseSet<Instruction *> UnlinkedInstructions;
};
----------------
broune wrote:
> 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?
Done.
================
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
----------------
broune wrote:
> to -> in
>
Done.
================
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;
----------------
broune wrote:
> 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.
Even if an add instruction is not used as an address by a load/store, it can be likely computed in one machine instruction (e.g., leal in x86). I agree this heuristic is inaccurate, but I don't have a better way of characterizing it. Any better idea?
================
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;
----------------
broune wrote:
> running forever -> taking too much time. It would terminate.
Done.
================
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);
----------------
broune wrote:
> Could add: so that it can be the basis of other candidates.
Done.
================
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);
----------------
broune wrote:
> Would it make sense to assert that I has exactly 2 operands?
Done.
================
Comment at: lib/Transforms/Scalar/StraightLineStrengthReduce.cpp:399
@@ -349,3 +398,3 @@
GetElementPtrInst *GEP) {
// At least, ArrayIdx = ArrayIdx *s 1.
allocateCandidateAndFindBasisForGEP(
----------------
broune wrote:
> is *s a typo?
Fixed. I mean `ArrayIdx = ArrayIdx *nsw 1`.
================
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).
----------------
broune wrote:
> would it make sense to catch the negative of a power of 2?
Done. Also added a test on that.
================
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);
----------------
broune wrote:
> Would it make sense to assert that Basis.Ins->getParent() != nullptr ?
Done.
http://reviews.llvm.org/D8983
EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/
More information about the llvm-commits
mailing list