[PATCH] Add a pass to transform aggregate loads and stores into scalar loads and stores.

Amaury SECHET deadalnix+llvmreview at gmail.com
Tue Mar 3 23:56:40 PST 2015


================
Comment at: lib/Transforms/Scalar/AggregateMemAccessRemoval.cpp:102
@@ +101,3 @@
+      InstrsToErase.push_back(LI);
+      runOnLoad(NewLI, InstrsToErase);
+    }
----------------
joker.eph wrote:
> deadalnix wrote:
> > deadalnix wrote:
> > > reames wrote:
> > > > This should be handled via an outer worklist not recursion, but please fix this as a separate change.
> > > > 
> > > > Actually, sorry, you're not testing this at all.  *Delete* this line and then let's revisit in another change.
> > > What is wrong with using recursion here ?
> > Also, this was tested in the whole diff. As only one element struct are supported now, I could write a test, but that would be very redundant with what already exists in D7607 .
> I don't like the approach "commit and fix in a separate change", unless there is a good reason and the "change" is already ready to follow. This is not a good process, and it build technical debt. Please do it right before committing.
> 
> Recursion is never a good idea unless you can prove that you won't explode the stack. LLVM is used in embedded environment where the stack size is limited. Even on a regular PC, you don't want to crash because you have a very long sequence of instruction and you recurse blindly.
Well ok but:
1/ There is the whole diff to begin with, I've been asked to cut a minimal chunk of it to get review. Now you got to understand that, because it is minimal, it won't contains the whole thhing, pretty much by definition. You guys needs to agree on what the process is here. Either we get it in in one piece or in several, but we can't have both at once.
2/ There is bunch of code that recurse on what aggregate type contains already. If this is going to stack overflow, this is going to with this recursion or not. Additionnaly, you'd have to be doign fairly bizantine stuff to get aggregate nested so deep that you get a stack overflow. You'd have to have something like a struct of a struct of a struct of a struct .... of one element enough time to get a stack overflow.

http://reviews.llvm.org/D7780

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/






More information about the llvm-commits mailing list