[PATCH] D50714: [InstCombine] Fold Select with binary op - FP opcodes

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineSelect.cpp:71
+  else if (FCmpInst::isEquality(Pred))
+    IsEq = Pred == FCmpInst::FCMP_OEQ || Pred == FCmpInst::FCMP_UEQ;
+  else
----------------
xbolva00 wrote:
> spatel wrote:
> > xbolva00 wrote:
> > > spatel wrote:
> > > > Did you step through the scenarios where X is NAN? I don't think the transform is valid for both predicates (similarly for ONE/UNE).
> > > Ok, so probably not worth to with FP opcodes? if it is so complicated.. Alive has some problems with float?
> > AFAIK, Alive doesn't support FP yet.
> > I think this is still a good optimization for FP, we just have to do it carefully. So that just means prove that the output is correct with a NAN input and one or more of the FP predicates.
> How can we prove it?
> 
> I will add many tests with nan tomorrow so we can see changes directly.
Options:
1. Use an FP-capable version of Alive - https://github.com/rutgers-apl/alive-nj (I have no experience with that).

2. Substitute %x with NaN in the basic pattern:

```
  %A = fcmp oeq float %x, -0.0
  %B = fadd float %x, %z
  %C = select i1 %A, float %B, float %y

```
And see if the result is the same after the transform by working through the code by hand.

3. Run this IR under 'lli'? (again, I'm not very familiar with that tool)

4. Create a C program that would produce the IR patterns and step/printf through to see if the output is the same given a NaN input.


================
Comment at: test/Transforms/InstCombine/select-binop-cmp.ll:361
+;
+  %A = fcmp oeq float %x, 0x7FF8000000000000
+  %B = fadd float %x, %z
----------------
These tests don't do what we need. If we're using a NaN constant (rather than a variable that is dynamically set to NaN), other optimizations will prevent the transform in question.


https://reviews.llvm.org/D50714