[PATCH] CSE on GEP indices

Thu Apr 24 11:28:25 PDT 2014

----- Original Message -----
> From: "Eli Bendersky" <eliben at google.com>
> To: "Hal Finkel" <hfinkel at anl.gov>
> Cc: reviews+D3462+public+cbb08f1e0c1f69e1 at reviews.llvm.org, llvm-commits at cs.uiuc.edu, "Jingyue Wu"
> <jingyue at google.com>, "justin holewinski" <justin.holewinski at gmail.com>
> Sent: Thursday, April 24, 2014 1:23:34 PM
> Subject: Re: [PATCH] CSE on GEP indices
> 
> 
> 
> 
> 
> 
> 
> On Thu, Apr 24, 2014 at 11:13 AM, Hal Finkel < hfinkel at anl.gov >
> wrote:
> 
> 
> 
> ----- Original Message -----
> > From: "Eli Bendersky" < eliben at google.com >
> > To: jingyue at google.com , hfinkel at anl.gov , eliben at google.com ,
> > "justin holewinski" < justin.holewinski at gmail.com >
> > Cc: llvm-commits at cs.uiuc.edu
> > Sent: Thursday, April 24, 2014 1:06:05 PM
> > Subject: Re: [PATCH] CSE on GEP indices
> > 
> > ================
> > Comment at: lib/Target/NVPTX/NVPTXTargetMachine.cpp:150
> > @@ -149,5 +149,3 @@
> > addPass(createNVPTXFavorNonGenericAddrSpacesPass());
> > - // The FavorNonGenericAddrSpaces pass may remove instructions and
> > leave some
> > - // values unused. Therefore, we run a DCE pass right afterwards.
> > We could
> > - // remove unused values in an ad-hoc manner, but it requires
> > manual work and
> > - // might be error-prone.
> > + addPass(createSeparateConstOffsetFromGEPPass());
> > + // The SeparateConstOffsetFromGEP pass creates variadic bases
> > that
> > can be used
> > ----------------
> > Since this pass is not target independent, I'm not sure this is the
> > right place to put it. It makes more sense to add it as part of a
> > custom compiler optimization pipeline, or maybe even in opt itself?
> 
> Why do you say this? As I see it, this pass is essentially LSR for
> the unrolled portion of multidimensional loop nests. And I asked
> them to add the TTI checks and move it because it is target
> independent. Because of the complex addressing modes available on
> x86, I'm not sure it will help there, but I suspect it will prove
> useful for several other targets (PPC, for example).
> 
> However, like LSR, it is not a canonicalization transformation, so it
> belongs, along with LSR, as a backend IR pass.
> 
> 
> 
> Fair enough. I just wanted to avoid code duplication among different
> passes, because NVPTX has its own addIRPasses. Another backend
> wanting to use this (PPC, for example) would need to duplicate this
> logic. Maybe it belongs in TargetPassConfig::addIRPasses() then ?

I'm not sure. We have some passes that interested targets are expected to add on their own (if conversion, for example). I don't have a strong opinion here, but I'm inclined to let the targets add the passes for now and then later, if we see a fairly-universal ordering, we can add things into TargetPassConfig with the necessary callbacks.

 -Hal

> 
> 
> Eli
> 
> 
> 
> -Hal
> 
> 
> 
> > 
> > Note that the other passes added here are in fact NVPTX specific.
> > Also the GVN vs. CSE logic seems out of place here. We already have
> > a place where optimization pipeline contents are customized
> > according to the optimization level - opt itself :)
> > 
> > We can add it under a off-by-default flag to PassManagerBuilder,
> > until others experiment with the pass. If it proves beneficial for
> > other archs, it may be enabled by default down the road.
> > 
> > ================
> > Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:1
> > @@ +1,2 @@
> > +//===-- SeparateConstOffsetFromGEP.cpp - -------------------*- C++
> > -*-===//
> > +//
> > ----------------
> > Inconsistent comment line length
> > 
> > ================
> > Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:98
> > @@ +97,3 @@
> > +
> > +static cl::opt<bool> DisableSplitGEP("disable-split-gep",
> > cl::init(false),
> > + cl::desc("Do not split GEPs for
> > CSE"),
> > ----------------
> > Make the flag name consistent with pass name
> > 
> > ================
> > Comment at:
> > lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:113
> > @@ +112,3 @@
> > +/// index, and tries to find a constant integer that can be
> > hoisted
> > to the
> > +/// outermost level of the expression as an addition. Not every
> > constants in an
> > +/// expression can jump out. e.g., we cannot transform (b * (a +
> > 5))
> > to (b * a +
> > ----------------
> > s/constants/constant/
> > 
> > ================
> > Comment at:
> > lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:121
> > @@ +120,3 @@
> > + /// numeric value of the extracted constant offset (0 if failed),
> > and a
> > + /// new index representing the remainder (equal to the original
> > index - the
> > + /// constant offset).
> > ----------------
> > Do you mean "minus" here? The dash is unclear as it can be just
> > part
> > of the sentence
> > 
> > ================
> > Comment at:
> > lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:130
> > @@ +129,3 @@
> > + Instruction *IP);
> > + static int64_t Find(Value *Idx, const DataLayout *DL);
> > +
> > ----------------
> > Document "Find"
> > 
> > ================
> > Comment at:
> > lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:204
> > @@ +203,3 @@
> > +INITIALIZE_PASS_BEGIN(
> > + SeparateConstOffsetFromGEP, "split-gep",
> > + "Split GEPs to a variadic base and a constant offset for better
> > CSE", false,
> > ----------------
> > Consistent name - separate / split
> > 
> > ================
> > Comment at:
> > lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:210
> > @@ +209,3 @@
> > +INITIALIZE_PASS_END(
> > + SeparateConstOffsetFromGEP, "split-gep",
> > + "Split GEPs to a variadic base and a constant offset for better
> > CSE", false,
> > ----------------
> > ditto
> > 
> > ================
> > Comment at:
> > lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:226
> > @@ +225,3 @@
> > + if (ConstantOffset != 0) return ConstantOffset;
> > + ConstantOffset = find(Usr->getOperand(1));
> > + // If Usr is a sub operator, negate the constant offset found in
> > the right
> > ----------------
> > Do you assert somewhere that users passed here have at least two
> > operands?
> > 
> > ================
> > Comment at:
> > lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:270
> > @@ +269,3 @@
> > + // If we found a non-zero constant offset, adds it to the path
> > for
> > future
> > + // analysis and surgery. Zero is a valid constant offset, but
> > doesn't help
> > + // this optimization.
> > ----------------
> > surgery :) ?
> > 
> > ================
> > Comment at:
> > lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:278
> > @@ +277,3 @@
> > +unsigned ConstantOffsetExtractor::FindFirstUse(User *U, Value
> > *Used)
> > {
> > + for (unsigned i = 0, e = U->getNumOperands(); i < e; ++i) {
> > + if (U->getOperand(i) == Used)
> > ----------------
> > s/i/I/ and s/e/E/
> > 
> > ================
> > Comment at:
> > lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:318
> > @@ +317,3 @@
> > + unsigned OpNo = FindFirstUse(U, C); // U->getOperand(OpNo) == C
> > + assert(OpNo != (unsigned)-1 && "UserChain wasn't built
> > correctly");
> > + Value *TheOther = U->getOperand(1 - OpNo); // The other operand
> > of
> > U
> > ----------------
> > Maybe asserting OpNo < 2 here is more inclusive?
> > 
> > ================
> > Comment at:
> > lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:383
> > @@ +382,3 @@
> > + gep_type_iterator GTI = gep_type_begin(*GEP);
> > + for (unsigned i = 1, e = GEP->getNumOperands(); i != e; ++i,
> > ++GTI) {
> > + if (isa<SequentialType>(*GTI)) {
> > ----------------
> > I, E
> > 
> > ================
> > Comment at:
> > lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:416
> > @@ +415,3 @@
> > + // trace into these casts.
> > + if (GEP->isInBounds()) {
> > + // Doing this to inbounds GEPs is safe because their indices are
> > guaranteed
> > ----------------
> > You can do this even if the offset doesn't need extraction? Is this
> > OK?
> > 
> > ================
> > Comment at:
> > lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:441
> > @@ +440,3 @@
> > + TargetTransformInfo &TTI = getAnalysis<TargetTransformInfo>();
> > + if (!TTI.isLegalAddressingMode(GEP->getType()->getElementType(),
> > + /*BaseGV=*/nullptr,
> > AccumulativeByteOffset,
> > ----------------
> > I think a comment here would be useful
> > 
> > ================
> > Comment at:
> > lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:450
> > @@ +449,3 @@
> > + gep_type_iterator GTI = gep_type_begin(*GEP);
> > + for (unsigned i = 1, e = GEP->getNumOperands(); i != e; ++i,
> > ++GTI) {
> > + if (isa<SequentialType>(*GTI)) {
> > ----------------
> > I, E
> > 
> > ================
> > Comment at:
> > lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:549
> > @@ +548,3 @@
> > + }
> > + return Changed;
> > +}
> > ----------------
> > Where do you modify Changed?
> > 
> > ================
> > Comment at:
> > test/Transforms/SeparateConstantOffsetFromGEP/NVPTX/gvn-pre.ll:1
> > @@ +1,2 @@
> > +; RUN: llc < %s -march=nvptx -mcpu=sm_20 | FileCheck %s
> > +; RUN: llc < %s -march=nvptx64 -mcpu=sm_20 | FileCheck %s
> > ----------------
> > Can you change the directory name to be consistent the pass name?
> > [I
> > mean s/Constant/Const/]
> > 
> > ================
> > Comment at:
> > test/Transforms/SeparateConstantOffsetFromGEP/NVPTX/gvn-pre.ll:2
> > @@ +1,3 @@
> > +; RUN: llc < %s -march=nvptx -mcpu=sm_20 | FileCheck %s
> > +; RUN: llc < %s -march=nvptx64 -mcpu=sm_20 | FileCheck %s
> > +
> > ----------------
> > Woudln't it be much more useful to test this on IR->IR level with
> > opt?
> > 
> > test/Transforms/LoopStrengthReduce/ has some examples of tests that
> > use opt with tripel flags to provide target info
> > 
> > http://reviews.llvm.org/D3462
> > 
> > 
> > 
> 
> --
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory
> 
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory