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

[Frits van Bommel]

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().

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.



P.S. While I can see how the sext/zext might not dominate the icmp, I
wonder how it's possible that some of those errors were for icmp
conditions not dominating selects since this code never touches the
condition operand of the select...