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

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

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.



================
Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:4162
+    bool Found = false;
+    for (Slice &S1 : P) {
+      if (!S1.isSplittable())
----------------
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.


================
Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:4164
+      if (!S1.isSplittable())
+        continue;
+      for (Slice &S2 : P) {
----------------
Is it possible that we don't end up actually splitting a partition after we pre-split?  Could we try harder to avoid that?


================
Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:4306
+    auto *V2 = V;
+    if (DL.isBigEndian()) {
+      uint64_t ShAmt = 8 * (StoreSize - PartSize);
----------------
Given this is endian-sensitive, I'd like to see a big-endian testcase.


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

https://reviews.llvm.org/D68414