[llvm-commits] [llvm] r52217 - in /llvm/trunk: lib/Transforms/Scalar/InstructionCombining.cpp test/Transforms/InstCombine/extractvalue.ll

[Dan Gohman]

On Wed, June 11, 2008 7:05 am, Matthijs Kooijman wrote:
> Author: matthijs
> Date: Wed Jun 11 09:05:05 2008
> New Revision: 52217
>
> URL: http://llvm.org/viewvc/llvm-project?rev=52217&view=rev
> Log:
> Teach instruction combining about the extractvalue. It can succesfully
> fold
> useless insert-extract chains, similar to how it folds them for  
> vectors.
>
> Add a testcase for this.

Hi Matthijs,

This looks good. Thanks for your help with insertvalue and
extractvalue! Review comments below.

> +    Value *FindScalarValue(Value *V,
> +                           const unsigned *idx_begin,
> +                           const unsigned *idx_end,
> +                           Instruction &InsertBefore);

The word "Scalar" here is a little confusing. Not only can it find
vectors (though vectors are scalars, for some uses of the word scalar),
but it also can find aggregates, if the indices don't lead all the way
to a leaf value. How about FindExtractedValue?

> +
> +// This helper takes a nested struct and extracts a part of it  
> (which is
> again a
> +// struct) into a new value. For example, given the struct:
> +// { a, { b, { c, d }, e } }
> +// and the indices "1, 1" this returns
> +// { c, d }.
> +//
> +// It does this by inserting an extractvalue and insertvalue for each
> element in
> +// the resulting struct, as opposed to just inserting a single  
> struct.
> This
> +// allows for later folding of these individual extractvalue  
> instructions
> with
> +// insertvalue instructions that fill the nested struct.
> +//
> +// Any inserted instructions are inserted before InsertBefore

It took me a bit of puzzling to figure out what BuildSubAggregate is
necessary. In many cases, it doesn't materially optimize the code,
since FindScalarValue knows how to dig through chains of
insertvalues and extractvalues, and it doesn't affect SelectionDAG-
using backends. But I think it does help expose portions of
struct values which are unneeded, which may be useful.

> +Value *InstCombiner::BuildSubAggregate(Value *From, const unsigned
> *idx_begin, const unsigned *idx_end, Instruction &InsertBefore) {

Here and in a few other places, watch out for going over 80 columns.

> +
> +    // Calculate the number of indices required
> +    unsigned size = I->getNumIndices() + (idx_end - idx_begin);
> +    // Allocate some space to put the new indices in
> +    unsigned *new_begin = new unsigned[size];
> +    // Auto cleanup this array
> +    std::auto_ptr<unsigned> newptr(new_begin);
> +    // Start inserting at the beginning
> +    unsigned *new_end = new_begin;

It's not permitted to use auto_ptr with arrays, since it uses delete and
not delete[]. Can you change this code to use a SmartVector instead?

> --- llvm/trunk/test/Transforms/InstCombine/extractvalue.ll (added)
> +++ llvm/trunk/test/Transforms/InstCombine/extractvalue.ll Wed Jun 11
> 09:05:05 2008
> @@ -0,0 +1,24 @@
> +; RUN: llvm-as < %s | opt -instcombine | llvm-dis | not grep
> extractelement

This should be extractvalue, not extractelement.

Dan