[PATCH] D70582: [FPEnv][X86] Constrained FCmp intrinsics enabling on X86

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/TargetLowering.h:179
+  /// Enum that signaling or quite index.
+  enum ActionIndex {
+    Quite = 1,      // Index of quite CC actions.
----------------
This enum name should mention compare or setcc or something its name. ActionIndex isn't going to read as being related to FP compares. Also, its not an Index, its a bit mask so using Index in the name is misleading


================
Comment at: llvm/include/llvm/CodeGen/TargetLowering.h:180
+  enum ActionIndex {
+    Quite = 1,      // Index of quite CC actions.
+    Signaling = 2,  // Index of Signaling CC actions.
----------------
Quite -> Quiet


================
Comment at: llvm/include/llvm/CodeGen/TargetLowering.h:2099
+                         LegalizeAction Action,
+                         ActionIndex AI = ActionIndex::Both) {
+    assert(VT.isValid() && (unsigned)CC < array_lengthof(CondCodeActions[0]) &&
----------------
Are we using the new support you've added here in this patch? If its not needed by X86 it doesn't belong in this patch.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:3560
+      if (IsStrict) {
+        DAG.ReplaceAllUsesOfValueWith(SDValue(Node,1), Chain);
+        ReplaceNodeWithValue(SDValue(Node, 0), Tmp1);
----------------
Can we add to the Results vector here instead of doing manual replacement?


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:20292
+                                   const SDLoc &dl, SelectionDAG &DAG,
+                                   SDValue Chain, bool IsSignaling) const {
   if (isNullConstant(Op1))
----------------
IsSignaling->IsSignalling


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:20865
+
+      // Insert an extra singaling instruction to raise exception.
+      if (IsStrict && !IsAlwaysSignaling && IsSignaling) {
----------------
singaling->signalling


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:20914
+      if (IsStrict)
+        // Make a flip on already singaling CCs before setting bit 4 of AVX CC.
+        SSECC |= (IsAlwaysSignaling ^ IsSignaling) << 4;
----------------
signaling->signalling


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:20926
     if (Opc == X86ISD::CMPP)
-      Cmp = DAG.getBitcast(Op.getSimpleValueType(), Cmp);
+      Cmp = DAG.getBitcast(Op->getSimpleValueType(0), Cmp);
+
----------------
Why is this change needed?


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:21313
+  SDValue Chain = IsStrict ? Op.getOperand(0) : SDValue();
+  SDValue Op0 = Op.getOperand(IsStrict ? 1: 0);
+  SDValue Op1 = Op.getOperand(IsStrict ? 2: 1);
----------------
Space before :


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:21317
+  ISD::CondCode CC
+                = cast<CondCodeSDNode>(Op.getOperand(IsStrict ? 3 : 2))->get();
 
----------------
Please run clang-format on this. I think the = should be at the end of the line above


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:21322
   if (Op0.getValueType() == MVT::f128) {
+    assert(!IsStrict && "Unhandled strict operation!");
     softenSetCCOperands(DAG, MVT::f128, Op0, Op1, CC, dl, Op0, Op1);
----------------
Is this something we need to fix? if so put a fixme here


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:21475
     SDValue CondOp0 = Cond.getOperand(0), CondOp1 = Cond.getOperand(1);
+    bool IsAlwaysSignaling;
     unsigned SSECC = translateX86FSETCC(
----------------
Signaling->Signalling


================
Comment at: llvm/test/CodeGen/X86/fp80-strict-scalar-cmp.ll:3
+; RUN: llc -disable-strictnode-mutation < %s -mtriple=i686-unknown-unknown -mattr=-sse -O3 | FileCheck %s --check-prefixes=CHECK,X87-32
+; RUN: llc -disable-strictnode-mutation < %s -mtriple=x86_64-unknown-unknown -mattr=-sse -O3 | FileCheck %s --check-prefixes=CHECK,X87-64
+
----------------
Is the explicit sse disable on the 64-bit line necessary?


================
Comment at: llvm/test/CodeGen/X86/vec-strict-128-cmp.ll:1
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -disable-strictnode-mutation < %s -mtriple=i686-unknown-unknown -mattr=+sse2 -O3 | FileCheck %s --check-prefixes=CHECK,SSE
----------------
There's no return instructions in your test checks, so I don't think this was truely generated with the script.


================
Comment at: llvm/test/CodeGen/X86/vec-strict-128-cmp.ll:9
+
+define <4 x i32> @f1(<4 x i32> %a, <4 x i32> %b, <4 x float> %f1, <4 x float> %f2) #0 {
+; SSE-LABEL: f1:
----------------
Can you name the test functions after what condition code and and signalling vs quiet? Same for all the new test files


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70582