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

[Mehdi Amini]

> On Mar 3, 2015, at 11:56 PM, Amaury SECHET <deadalnix+llvmreview at gmail.com> wrote:
> 
> ================
> 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.

I don’t understand how this part applies to my comment in particular.

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

I was answering to "This should be handled via an outer worklist not recursion, but please fix this as a separate change.”, and "What is wrong with using recursion here ?”.
So, take it as a generic statement. A recursion might be harmless here, but since I had to fix two places in LLVM the last months where we crashed because of a recursion, so I’d rather take a careful approach. 
Even a comment like “// Recursion is bounded by the nested level of the types” would be fine for me, it shows at least that the code was analyzed with respect to the recursion depth problem.

Best,

Mehdi