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

My biggest concern with this patch is that you're blindly rewriting based on the type of the alloca.  That type isn't always reliable, particularly when you're dealing with unions.  And even in cases where it is reliable, it might not reflect the actual operations the code ends up using in practice.  I'm concerned you'll end up preemptively splitting operations that didn't need to be split, and hurt performance by doing that.

If we're going to do a transform like this, I'd like to see the following:

1. Base the transform on the SROA slices rather than the IR type.  IR types of memory are not trustworthy; the true shape should be based on actual usage.
2. Specifically target memory operations where splitting them would unblock mem2reg.  Otherwise it isn't obvious the transform is profitable.  Partitions which can't be transformed to registers aren't that common, but various operations can trigger this: volatile operations, operations behind a select, etc.



================
Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:3586
 
-  bool visitPHINode(PHINode &PN) {
+  void visitPHINode(PHINode &PN) {
     enqueueUsers(PN);
----------------
Rearranging functions like this makes it harder to read the patch.


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

https://reviews.llvm.org/D68414