[llvm-commits] [llvm] r123034 - in /llvm/trunk: lib/Transforms/InstCombine/InstCombineSelect.cpp test/Transforms/InstCombine/select.ll
grosser at fim.uni-passau.de
Sun Jan 9 10:43:57 CST 2011
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
> - 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().
> In the comment above this you should probably replace "edit ICI" with
> "move ICI or edit it".
> 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
More information about the llvm-commits