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

Your reply doesn't really address the most important point.  The entire premise of your extractTypeFields is that the type of an alloca is a reliable guide to the way code will access that alloca.  That simply is not true: clang cannot generate appropriate LLVM types in some cases.  So you'll end up with weird behaviors in many cases.  I understand that the patch tries to recognize certain cases, but it still doesn't address the fundamental problem.

Instead of extracting fields from the type, I expect the code to construct "fields" based on actual memory accesses.



================
Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:3586
 
-  bool visitPHINode(PHINode &PN) {
+  void visitPHINode(PHINode &PN) {
     enqueueUsers(PN);
----------------
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.


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

https://reviews.llvm.org/D68414