[PATCH] D68414: [SROA] Enhance AggLoadStoreRewriter to rewrite integer load/store if it covers multi fields in original aggregate

[Guozhi Wei via Phabricator via llvm-commits]

Carrot marked 3 inline comments as done.
Carrot added a comment.

In D68414#2035503 <https://reviews.llvm.org/D68414#2035503>, @efriedma wrote:

> I'm happy with the general approach here; just some questions about the details.
>
> Also, can you gather numbers about how frequently this triggers over some testsuite like the LLVM testsuite or SPEC?  I'd like some idea about how large the impact of this is going to be.


For LLVM testsuite function presplitOverlappedSlices returns true for 114 times, I think most of them are from basictest.ll.



================
Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:4162
+    bool Found = false;
+    for (Slice &S1 : P) {
+      if (!S1.isSplittable())
----------------
efriedma wrote:
> I think this is worst-case O(n^3) over the number of operations in a partition?  There's one loop outside the function, and two loops inside the function.  I guess it's unlikely to bite in most cases, but it's the sort of thing that can easily blow up to infinite compile-time.
I limit the number of calls to this function to some constant.


================
Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:4164
+      if (!S1.isSplittable())
+        continue;
+      for (Slice &S2 : P) {
----------------
efriedma wrote:
> Is it possible that we don't end up actually splitting a partition after we pre-split?  Could we try harder to avoid that?
This function only presplits slices, it doesn't split partitions. The partitions are only constructed on demand when we request an iterator from AllocaSlices like:
    for (auto &P : AS.partitions())


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68414/new/

https://reviews.llvm.org/D68414