[llvm-commits] [llvm] r123034 - in /llvm/trunk: lib/Transforms/InstCombine/InstCombineSelect.cpp test/Transforms/InstCombine/select.ll

[Tobias Grosser]

On 01/09/2011 04:17 AM, Frits van Bommel wrote:
> On Sun, Jan 9, 2011 at 9:04 AM, Tobias Grosser
> <grosser at fim.uni-passau.de>  wrote:
>> On 01/09/2011 01:46 AM, Cameron Zwarich wrote:
>>> Even with r123061, I still see the following on one of my tests against
>>> SPEC. I guess I should make a reduction for you guys.
>>
>> I believe the attached patch should fix the issue. Could you give it a try?
>>
>> I am not 100% sure, if this is the right approach so a review before
>> committing this would be appreciated.
>
> -//===- InstCombineSelect.cpp
> ----------------------------------------------===//
> -//
>
> *ahem*
>
> -        ICI->setPredicate(Pred);
> -        ICI->setOperand(0, CmpLHS);
> -        ICI->setOperand(1, CmpRHS);
>           SI.setOperand(1, TrueVal);
>           SI.setOperand(2, FalseVal);
> +
> +        // Move ICI instruction right before the select instruction. Otherwise
> +        // the sext/zext value may be defined after the ICI instruction uses
> +        // it.
> +        ICmpInst *newICI = new ICmpInst(&SI, Pred, CmpLHS, CmpRHS);
> +        ReplaceInstUsesWith(*ICI, newICI);
> +        StringRef ICIName = ICI->getName();
> +        EraseInstFromFunction(*ICI);
> +        newICI->setName(ICIName);
>
> Instead of creating a whole new ICmp in a new place and deleting the
> old one, you can keep the updating code and use ICI->moveBefore(SI).
> This should have the same effect but be much cheaper.
> For future reference: Value::takeName() is probably more efficient
> than getName() + setName().
Good point.

>
> In the comment above this you should probably replace "edit ICI" with
> "move ICI or edit it".
Done.


> The only case I can think of where moving the icmp might be
> undesirable would be if it was a loop-invariant instruction and the
> extension operation already dominated it (or it would be cheaper to
> move/copy the extension to just before the icmp). I don't think this
> is a very important case, especially since the first few runs of
> -instcombine are followed by loop passes that should clean this up.
OK. I kept the behavior of the patch. We can improve it if this shows to
be important.

Committed in:
http://llvm.org/viewvc/llvm-project/?view=rev&revision=123121

Cheers
Tobi