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

[Jun Bum Lim via Phabricator via llvm-commits]

junbuml added a comment.

Just minor nits inlined.



================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:3746
+      } else {
+        // 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.
----------------
a non-zero offsets  -> a non-zero offset


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:3752
+        // address for the memory access.
+        if (EnableGEPOffsetSplit && isa<GetElementPtrInst>(AddrInst) &&
+            TLI.shouldConsiderGEPOffsetSplit(Base->getType()) && Depth == 0 &&
----------------
It seems that you can move this check to "else" by changing "else"  to "else if".


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4293
 
+    if (LargeOffsetGEP.first &&
+        LargeOffsetGEP.first->getParent() != MemoryInst->getParent() &&
----------------
You may want to have a new variable for GetElementPtrInst  instead of doing LargeOffsetGEP.first several times.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4295
+        LargeOffsetGEP.first->getParent() != MemoryInst->getParent() &&
+        NewGEPBases.count(LargeOffsetGEP.first) == 0)
+      // Only collect GEPs that cannot be sunk unless the underlying data
----------------
!NewGEPBases.count(LargeOffsetGEP.first)


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4297
+      // Only collect GEPs that cannot be sunk unless the underlying data
+      // structures are split and not the bases of previously spllit GEPs.
+      LargeOffsetGEPMap[LargeOffsetGEP.first->getPointerOperand()].push_back(
----------------
I'm not clear about this comment. Can you clarify it little bit? 

typo: spllit 


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:8033
 
+bool AArch64TargetLowering::shouldConsiderGEPOffsetSplit(Type *BaseType) const {
+  // Consider splitting large offset of struct or array.
----------------
Should we have BaseType as parameter ? 


Repository:
  rL LLVM

https://reviews.llvm.org/D42759