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

Consider something like the following C testcase:

  union U {
    struct A {
      long m1;
      int m2;
    } a;
    struct B {
      int m;
      struct C {
        int z[2];
      } c;
    } b;
  };
  
  void f(struct C);
  void a(struct C c) {
    union U x = { .b = { 1, c } };
    f(x.b.c);
  }

The alloca is generated with a type like "{i64, i32}", but all the accesses happen as it it were "{i32, i64}".  If you assume "{i64, i32}" actually describes the memory accesses, you're paying the price for extra splitting with no profit.  (In this particular case, we might eventually recover in instcombine, but more generally the optimization is going to be unpredictable.)



================
Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:3586
 
-  bool visitPHINode(PHINode &PN) {
+  void visitPHINode(PHINode &PN) {
     enqueueUsers(PN);
----------------
Carrot wrote:
> efriedma wrote:
> > Carrot wrote:
> > > efriedma wrote:
> > > > Rearranging functions like this makes it harder to read the patch.
> > > This is because the base class is changed from InstVisitor to PtrUseVisitor. These 2 classes have different return types for various visit functions. Fortunately they have similar semantics.
> > I don't see the relationship between changing the base class and reordering the visit* functions.  It should be possible to write the functions in any order for either base class.
> I didn't reorder any visit* functions. I deleted some visit* functions because PtrUseVisitor has same default implementations.
> 
> Phabricator diff algorithm matches the meaningless "}" and blank lines, causes the result looks strange.
Oh, sorry, you're right, not your fault.


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

https://reviews.llvm.org/D68414