[PATCH] Add a pass to transform aggregate loads and stores into scalar loads and stores.
mehdi.amini at apple.com
Tue Mar 3 22:27:40 PST 2015
Comment at: lib/Transforms/Scalar/AggregateMemAccessRemoval.cpp:102
@@ +101,3 @@
+ runOnLoad(NewLI, InstrsToErase);
> 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.
More information about the llvm-commits