[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.

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

I mean the LLVM testsuite, not the regression tests.  (http://llvm.org/docs/TestSuiteGuide.html).  I'm trying to get an idea how wide the impact is on general C/C++ code.  I'm not expecting this to trigger very frequently, but I want to make sure my intuition is correct.



================
Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:4164
+      if (!S1.isSplittable())
+        continue;
+      for (Slice &S2 : P) {
----------------
Carrot wrote:
> 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())
I wasn't trying to ask about the partitions datastructure.

I meant, are there cases where we do the transform, but then SROA can't do any further transforms?  So instead of a single load, we have two loads and some bit manipulation that doesn't simplify?  I'd like to avoid splitting the load/store instruction if it''s not actually going to help.


================
Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:4141
+// Limit the number of times presplitOverlappedSlices is called.
+#define MAX_PRESPLIT_ITERATIONS 128
+
----------------
`static const int MaxPresplitIterations = 128;`


================
Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:4356
+
+  // Remove the killed slices that have ben pre-split.
+  AS.erase(llvm::remove_if(AS, [](const Slice &S) { return S.isDead(); }),
----------------
"been"


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

https://reviews.llvm.org/D68414