[llvm-commits] [Review request] Instcombine: extractvalue from load --> load from gep

[Chris Lattner]

On Nov 29, 2010, at 2:21 PM, Frits van Bommel wrote:

> On Mon, Nov 29, 2010 at 10:30 PM, Chris Lattner <clattner at apple.com> wrote:
>> I think that Dan is pointing out that it is unwise for an frontend to generate a ton of aggregate values, because that will cause fastisel to be defeated and lead to slower compile times.  However, despite Dan's objections, I think this is a perfectly reasonable xform for instcombine to do.
> 
> I agree that it's probably not a very good thing to do, but apparently
> they do occur "in the wild" even with clang.

Yep, clang generates them on x86-64 for ABI reasons.

> Also, I've found aggregate values can really make a simple frontend
> easier to write sometimes when you can just assume that any expression
> will generate (at most) a single llvm::Value* even if the result
> logically has two fields, e.g. a pointer and length for a dynamic
> array.
> So if you prefer ease of implementation, this is at least a useful hack.

Completely agreed.  It's also useful for complex numbers and a variety of other things.

> One way extractvalues can enter the code, by the way, is return values
> of functions. For instance, on x86-64 9-16 byte structs are returned
> as a 2-field struct value. When such a function gets inlined, this
> optimization may fire if the frontend used a single load + ret instead
> of two loads + two insertvalues + ret to construct the return value.
> (The fact that using more instructions causes it to optimize better
> can be a bit counter-intuitive)

Yep.

> We could also add a "load %struct*" --> load fields + construct
> %struct with insertvalues transform...

Not a bad idea, though probably not worth it.  At -O0 (when fastisel kicks in) no optimization passes are run :)

> That's what the first version did, and the exact reason it was
> horribly broken :). Doing that causes -instcombine to try to insert it
> at the current location, which is that of the extractvalue. Adding an
> explicit insert point causes an assert when trying to insert it
> because it's already in a basic block.

Thanks again Frits,

-Chris