[PATCH] CSE on GEP indices
Eli Bendersky
eliben at google.com
Thu Apr 24 11:23:34 PDT 2014
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 ?
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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140424/24e4067d/attachment.html>
More information about the llvm-commits
mailing list