[PATCH] D42759: [CGP] Split large data structres to sink more GEPs

[Serguei Katkov via Phabricator via llvm-commits]

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