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

[Pengfei Wang via Phabricator via llvm-commits]

pengfei marked 2 inline comments as done.
pengfei 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);
----------------
craig.topper wrote:
> 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
Done. Does it matter for the push order?


================
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:
> 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
Right! I added it back. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70582