[PATCH] D69281: [FPEnv][WIP] Constrained FCmp intrinsics

[Cameron McInally via Phabricator via llvm-commits]

cameron.mcinally added a comment.

> I did not actually require any TableGen changes, but was able to represent the overloaded intrinsic types using existing mechanisms. This solution looks correct to me, but if I'm missing anything here, please let me know ...

That sounds right. `LLVMScalarOrSameVectorWidth` did not exist when D54649 <https://reviews.llvm.org/D54649> was proposed, IINM.

> This adds a full set of compare intrinsics for all comparison codes.

There's really no reasonable alternative, correct? I believe that negating `<` and `>` would get messy wrt signals when a NaN is present. I don't think that savings is worth the effort (e.g. readability).

> The X86 back-end changes are incomplete, they're right now the bare minimum to keep Cameron's two test cases working.

I'd be ok with leaving these changes to a later patch. Your call...



================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:7073
   SDVTList VTs = DAG.getVTList(ValueVTs);
-  SDValue Result;
-  if (Opcode == ISD::STRICT_FP_ROUND)
-    Result = DAG.getNode(Opcode, sdl, VTs,
-                          { Chain, getValue(FPI.getArgOperand(0)),
-                               DAG.getTargetConstant(0, sdl,
-                               TLI.getPointerTy(DAG.getDataLayout())) });
-  else if (FPI.isUnaryOp())
-    Result = DAG.getNode(Opcode, sdl, VTs,
-                         { Chain, getValue(FPI.getArgOperand(0)) });
-  else if (FPI.isTernaryOp())
-    Result = DAG.getNode(Opcode, sdl, VTs,
-                         { Chain, getValue(FPI.getArgOperand(0)),
-                                  getValue(FPI.getArgOperand(1)),
-                                  getValue(FPI.getArgOperand(2)) });
-  else
-    Result = DAG.getNode(Opcode, sdl, VTs,
-                         { Chain, getValue(FPI.getArgOperand(0)),
-                           getValue(FPI.getArgOperand(1))  });
+  SDValue Result = DAG.getNode(Opcode, sdl, VTs, Opers);
 
----------------
These `DAG.getNode` changes could probably be broken out into a standalone Diff.


================
Comment at: lib/Target/SystemZ/SystemZISelLowering.cpp:2171
+  if (C.Chain)
+    return;
   auto *C1 = dyn_cast<ConstantFPSDNode>(C.Op1);
----------------
This is probably worthy of a comment.


================
Comment at: lib/Target/SystemZ/SystemZISelLowering.cpp:2655
+    case CmpMode::FP:       return 0;
+    case CmpMode::StrictFP: return 0;
+    default:                llvm_unreachable("Bad mode");
----------------
I notice the `static_cast<SystemZISD::NodeType>` has been dropped. Is this ok?


================
Comment at: lib/Target/SystemZ/SystemZISelLowering.h:248
 
   // Compare floating-point vector operands 0 and 1 to preoduce the usual 0/-1
   // vector result.  VFCMPE is for "ordered and equal", VFCMPH for "ordered and
----------------
`preoduce` typo could use a fix.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69281/new/

https://reviews.llvm.org/D69281