[PATCH] D42101: [PowerPC] Legalize SETULT and SETUGT for type f32 and f64.

[Tony Jiang via Phabricator via llvm-commits]

jtony added inline comments.


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:3521
   case ISD::SETUO:  return PPC::PRED_UN;
-    // These two are invalid for floating point.  Assume we have int.
   case ISD::SETULT: return PPC::PRED_LT;
----------------
nemanjai wrote:
> I don't think this is safe to just overlook. Sure the comment doesn't apply any more, but the solution does not appear to be to just remove the comment. You'll have to see where the enclosing function is called and what assumptions are based on us not having FP here.
> At the very least, one example shows that we'll have a miscompile due to this:
> ```
> define double @test(double %a, double %b, double %c, double %d) {
> entry:
>   %cmp = fcmp ult double %a, %b
>   %cond = select i1 %cmp, double %c, double %d
>   ret double %cond
> }
> ```
> That'll produce a `SET_CC_VSFRC` from the `ISD::SELECT_CC` and the predicate will be `PPC::PRED_LT`. Of course, that won't consider the `UN` bit. And sure enough, it'll produce the following code:
> ```
>         xscmpudp 0, 1, 2
>         blt     0, .LBB0_2
> ```
> when it should actually be producing:
> ```
>         xscmpudp 0, 1, 2
>         cror 20, 0, 3
>         bc 12, 20, .LBB0_2
> ```
That's a very good catch. I am trying to figure out how we should fix this wrong code-gen here.  BTW, one minor nit in the comment: the code you provided will actually produce a `SELECT_CC_VSFRC` from the ISD::SELECT_CC (we don't have `SET_CC_VSFRC`)


https://reviews.llvm.org/D42101