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

[Guozhi Wei via Phabricator via llvm-commits]

Carrot marked an inline comment as done.
Carrot added a comment.

In D68414#1978709 <https://reviews.llvm.org/D68414#1978709>, @efriedma wrote:

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


Could you show me an example?

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

For the following instruction in the attached test case the actual memory access is 64bit integer load. This is exactly the problematic instruction and I want to split it.

%split = load i64, i64* %altptr, align 8



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


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

https://reviews.llvm.org/D68414