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

[Chris Lattner]

On Nov 28, 2010, at 2:43 AM, Frits van Bommel wrote:

> On Sun, Nov 28, 2010 at 3:52 AM, Frits van Bommel <fvbommel at gmail.com> wrote:
>> No, definitely not okay. It inserts the new load at the location of
>> the extractvalue instead of the location of the old load. Fixed
>> version attached. I'll provide an extra test case once I've had some
>> sleep...
> 
> So apparently 4 AM is not the best time to try to fix bugs. Who knew?
> I forgot to even run the testcase.
> That version is even more broken than the one it replaced; it asserts
> every time it fires.
> 
> Here's the real fix, as well as the promised adjusted testcase that
> checks for the original problem.

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.  

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.

Thanks Frits!

-Chris