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

[Pengfei Wang via Phabricator via llvm-commits]

pengfei marked 12 inline comments as done.
pengfei added a comment.

Thanks for the review!



================
Comment at: llvm/include/llvm/CodeGen/TargetLowering.h:2099
+                         LegalizeAction Action,
+                         ActionIndex AI = ActionIndex::Both) {
+    assert(VT.isValid() && (unsigned)CC < array_lengthof(CondCodeActions[0]) &&
----------------
craig.topper wrote:
> 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.
Yes. We are using it for CCs `SETOEQ` and `SETUNE` in all scalar FP types.


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


================
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);
+
----------------
craig.topper wrote:
> Why is this change needed?
They have the same meaning here. Changes reverted.


================
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);
----------------
craig.topper wrote:
> Is this something we need to fix? if so put a fixme here
Removed.
I was intending to enable fp128. But it wouldn't be enabled in this patch now.


================
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
+
----------------
craig.topper wrote:
> Is the explicit sse disable on the 64-bit line necessary?
Removed, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70582