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

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:2548
+  // A GEP which has too large offset to be folded into the addressing mode.
+  std::pair<GetElementPtrInst *, int64_t> &LargeOffsetGEP;
+
----------------
`AssertingVH<GetElementPtrInst>`


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4892
+    Value *NewBaseGEP = nullptr;
+    for (auto &LargeOffsetGEP : LargeOffsetGEPs) {
+      GetElementPtrInst *GEP = LargeOffsetGEP.first;
----------------
This is going to visit GEPs with the same offset in non-deterministic order.  I guess it might not be that important, but it'll probably mess with the use-lists, which could affect some later pass.  I would rather stay on the safe side and sort GEPs with the same offset based on the order they were added to the map, or some other deterministic ordering.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4899
+        if (!TLI->isLegalAddressingMode(*DL, AddrMode,
+                                        GEP->getResultElementType(),
+                                        GEP->getAddressSpace())) {
----------------
The result type of the GEP might not be the type of the memory access... but I don't know how you'd get the right type off the top of my head, and it might not be important in practice.  Maybe worth noting with a comment, though.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:4933
+          } else
+            NewBaseInsertPt = std::next(BaseI->getIterator());
+        } else {
----------------
I think you have to be a bit more careful here to avoid inserting a GEP into a block with a catchswitch.  (This is Windows-only exception-handling, so maybe difficult to trigger.)


Repository:
  rL LLVM

https://reviews.llvm.org/D42759