[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/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:3560
+      if (IsStrict) {
+        DAG.ReplaceAllUsesOfValueWith(SDValue(Node,1), Chain);
+        ReplaceNodeWithValue(SDValue(Node, 0), Tmp1);
----------------
pengfei wrote:
> craig.topper wrote:
> > Can we add to the Results vector here instead of doing manual replacement?
> I think so. Thanks!
Don’t use merge values, just push both results to the vector


================
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);
----------------
pengfei wrote:
> 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.
But we do need to change this code eventually right? So an assert will help us get an obvious crash and a FIXME will help us audit the code


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70582