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

[Frits van Bommel]

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.

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.

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)


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

> A comment on the patch:
>
> +      return ReplaceInstUsesWith(EV, Builder->CreateLoad(GEP));
>
> Please just use: "return new LoadInst(GEP);", assuming that it will be inserted in the correct place.  If not, then your code is fine with an added comment explaining what is going on.

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.

I changed the last bit to
      // We need to insert these at the location of the old load, not at that of
      // the extractvalue.
      Builder->SetInsertPoint(L->getParent(), L);
      Value *GEP = Builder->CreateInBoundsGEP(L->getPointerOperand(),
                                              Indices.begin(), Indices.end());
      // Returning the load directly will cause the main loop to insert it in
      // the wrong spot, so use ReplaceInstUsesWith().
      return ReplaceInstUsesWith(EV, Builder->CreateLoad(GEP));

(only the comments have changed)

Committed as r120323.