[PATCH] Optimize redundant insertvalue instructions

[Michael Zolotukhin]

On May 7, 2014, at 5:03 PM, Hal Finkel <hfinkel at anl.gov> wrote:

> ----- Original Message -----
>> From: "Michael Zolotukhin" <mzolotukhin at apple.com>
>> To: "Hal J. Finkel" <hfinkel at anl.gov>
>> Cc: "LLVM Commits" <llvm-commits at cs.uiuc.edu>
>> Sent: Wednesday, May 7, 2014 7:07:35 AM
>> Subject: Re: [PATCH] Optimize redundant insertvalue instructions
>> 
>> 
>> 
>> Hi Hal,
>> 
>> Thank you for the review, I updated the patch accordingly to your
>> remarks.
>> 
>> There is a more general alternative to the cut-off: we could move
>> from uses to defs, maintaining a bitvector, that indicates which
>> fields were written. In this case the complexity would be O(N) and
>> we will also catch partially overlapping accesses as well. I did
>> something like this in my ‘sophisticated’ implementation.
>> 
>> However, I’d prefer the most simple implementation, so I think that
>> cut-off would be the best option here. I chose limit 10 - I don’t
>> expect long chains of insertvalue instructions anyway, and it is
>> enough for the original test.
> 
> Fair enough.
> 
>> 
>> Here is the new patch:
>> 
>> 
> 
> I don't actually see a call to visitInsertValueInst being inserted by the patch; I suppose you're putting it somewhere appropriate or else the test case would fail, right? Assuming that's the case, LGTM.
> 
Thanks.
There is no need to explicitly call visitInsertValueInst. We use InstVisitor interface (there is a call ‘visit’ in InstCombiner::DoOneIteration). The function ‘visit' is a big switch, constructed by macroses from IR/Instruction.def, and once we override visitInsertValueInst, it starts to be called. That’s how I understand this - if I’m mistaken I’d appreciate any corrections here. And yes, the test will fail otherwise.

Michael

> -Hal
> 
>> 
>> 
>> 
>> Thanks,
>> Michael
>> 
>> On May 7, 2014, at 3:01 PM, Hal Finkel <hfinkel at anl.gov> wrote:
>> 
>>> Michael,
>>> 
>>> +/// Here the second instruction inserts values at the same
>>> indices, as the
>>> 
>>> +/// first one, making it redundant.
>>> 
>>> You mean, "making the first one redundant."
>>> 
>>> + Value *V = dyn_cast<Value>(&I);
>>> 
>>> There should be no cast necessary here (and it certainly shouldn't
>>> be a dyn_cast).
>>> 
>>> + V = dyn_cast<Value>(UserInsInst);
>>> 
>>> Same here.
>>> 
>>> + while (V->hasOneUse()) {
>>> 
>>> Should we have a cut-off here? If this function is called on every
>>> InsertValue instruction, then this algorithm will be O(N^2) in the
>>> size of the InsertValue chain.
>>> 
>>> -Hal
>>> 
>>> ----- Original Message -----
>>>> From: "Michael Zolotukhin" <mzolotukhin at apple.com>
>>>> To: "LLVM Commits" <llvm-commits at cs.uiuc.edu>
>>>> Sent: Tuesday, May 6, 2014 9:42:12 AM
>>>> Subject: [PATCH] Optimize redundant insertvalue instructions
>>>> 
>>>> 
>>>> Hi,
>>>> 
>>>> 
>>>> This patch implements a simple optimization of redundant
>>>> insertvalue
>>>> instructions.
>>>> 
>>>> 
>>>> Here is a small example where it will help:
>>>> 
>>>> %0 = insertvalue { i8, i32 } undef, i8 %x, 0
>>>> %1 = insertvalue { i8, i32 } %0, i8 %y, 0
>>>> 
>>>> 
>>>> In this example, the second instruction inserts values at the same
>>>> indices, as the first one, and consequently, the first instruction
>>>> is redundant.
>>>> This code will be transformed to:
>>>> 
>>>> %0 = insertvalue { i8, i32 } undef, i8 %y, 0
>>>> 
>>>> 
>>>> This optimization catches nothing on specs and test-suite, but the
>>>> example was inspired by a real world application.
>>>> 
>>>> 
>>>> As an experiment, I tried a more sophisticated implementation,
>>>> which
>>>> was capable of dealing with several users and with partially
>>>> overlapping indices, but that also caught nothing on specs (and
>>>> for
>>>> my application the simple version was enough). If there is an
>>>> interest, I could share a patch with this more complicated
>>>> implementation as well.
>>>> 
>>>> 
>>>> Is it ok to commit this patch?
>>>> 
>>>> 
>>>> 
>>>> 
>>>> 
>>>> 
>>>> 
>>>> Thanks,
>>>> Michael
>>> 
>>> --
>>> Hal Finkel
>>> Assistant Computational Scientist
>>> Leadership Computing Facility
>>> Argonne National Laboratory
>> 
>> 
> 
> -- 
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory