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

[Chad Rosier via Phabricator via llvm-commits]

mcrosier added a comment.

A few quick comments.  Will follow up with a more complete review later this week.



================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:3743
+      } else {
+        // Record a GEP which has too large offset that cannot fit into the r+i
+        // addressing mode.  The GEP is the candidate to be split later.
----------------
Perhaps something like:

  // Record GEPs with a non-zero offsets as candidates for splitting in the event that the offset cannot fit into the r+i addressing mode.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:3746
+        Value *Base = AddrInst->getOperand(0);
+        if (EnableGEPOffsetSplit &&
+            TLI.shouldConsiderGEPOffsetSplit(Base->getType()) && Depth == 0 &&
----------------
Add  isa<GetElementPtrInst>(AddrInst) check?


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:3755
+               !isa<GetElementPtrInst>(BaseI)))
+            if (auto *GEP = dyn_cast<GetElementPtrInst>(AddrInst))
+              if ((BaseI && GEP->getParent() != BaseI->getParent()) ||
----------------
You could reduce the indent here by putting a 'isa<GetElementPtrInst>(AddrInst) in the outer most if statement and then make this a cast<>.  See comment above.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:3756
+            if (auto *GEP = dyn_cast<GetElementPtrInst>(AddrInst))
+              if ((BaseI && GEP->getParent() != BaseI->getParent()) ||
+                  (!BaseI &&
----------------
Also, I need some clarification here.  The first check

  (BaseI && GEP->getParent() != BaseI->getParent())

seems to be inline with the comments below, but I don't completely follow the second check (i.e., checking that the PHI is not in the entry block, if BaseI is null).


Repository:
  rL LLVM

https://reviews.llvm.org/D42759