[PATCH] D42759: [CGP] Split large data structres to sink more GEPs
Serguei Katkov via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Mar 26 22:12:21 PDT 2018
skatkov added inline comments.
================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:3757
return true;
+ } else if (EnableGEPOffsetSplit && isa<GetElementPtrInst>(AddrInst) &&
+ TLI.shouldConsiderGEPOffsetSplit() && Depth == 0 &&
----------------
Why do you need the check isa<GetElementPtrInst>(AddrInst)?
You are in "case Instruction::GetElementPtr" basing on "switch (Opcode) {" where Opcode is actually AddrInst->getOpcode().
So to me it is redundant...
================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:3776
+ BaseI ? BaseI->getParent()
+ : &GEP->getParent()->getParent()->getEntryBlock();
+ if (GEP->getParent() != Parent)
----------------
getParent()->getParent() => getFunction()
================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4819
+static int
+compareGEPOffset(const std::pair<GetElementPtrInst *, int64_t> *LHS,
----------------
Please add a comment for nontrivial comparison function.
================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4873
+ compareGEPOffset);
+ LargeOffsetGEPs.erase(
+ std::unique(LargeOffsetGEPs.begin(), LargeOffsetGEPs.end()),
----------------
I wonder if you sort the an array anyway why not to use set like data-structure to keep them and avoid erase of unique elements?
Is there any hidden sense for that?
================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4885
+ int64_t Offset = LargeOffsetGEP.second;
+ if (!GEP)
+ continue;
----------------
How can we get GEP == nullptr here?
================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4929
+ // will be inserted to the entry block.
+ NewBaseInsertBB = &BaseGEP->getParent()->getParent()->getEntryBlock();
+ NewBaseInsertPt = NewBaseInsertBB->getFirstInsertionPt();
----------------
getParent()->getParent() => getFunction()
Repository:
rL LLVM
https://reviews.llvm.org/D42759
More information about the llvm-commits
mailing list