[PATCH] D14079: [FPEnv Core 14/14] Introduce F*_W_CHAIN instrs to prevent reordering

[Mehdi AMINI via llvm-commits]

joker.eph added a comment.

Haven't looked at the target specific code, seems mostly mechanical changes. 
Some comments on the generic part.


================
Comment at: include/llvm/CodeGen/SelectionDAG.h:1228
@@ -1225,5 +1227,3 @@
   /// on each element individually.  If the ResNE is 0, fully unroll the vector
-  /// op. If ResNE is less than the width of the vector op, unroll up to ResNE.
-  /// If the  ResNE is greater than the width of the vector op, unroll the
-  /// vector op and fill the end of the resulting vector with UNDEFS.
-  SDValue UnrollVectorOp(SDNode *N, unsigned ResNE = 0);
+  /// op.  If ResNE is less than the width of the vector op, unroll up to ResNE.
+  /// If the ResNE is greater than the width of the vector op, unroll the
----------------
Remove spurious space change.

================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:1338
@@ +1337,3 @@
+      SDValue OpV[] = { RV, DAG.getEntryNode() };
+      DAG.ReplaceAllUsesWith(N, OpV);
+    } else {
----------------
I'm not sure I understand what's going on here, this need to be documented.

================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:8701
@@ +8700,3 @@
+                         Chain, N0, RV, Flags);
+    }
+  }
----------------
It is not clear to me that this will preserve the FP env?
Also "UnsafeFPMath" seems fundamentally not compatible with FP env, so it smells.

================
Comment at: lib/CodeGen/SelectionDAG/LegalizeTypes.cpp:1064
@@ -1063,2 +1063,3 @@
   SDLoc dl(N);
+
   if (NumOps == 0) {
----------------
spurious change

================
Comment at: lib/CodeGen/SelectionDAG/LegalizeTypes.cpp:1070
@@ -1068,4 +1069,3 @@
     SDValue Op = N->getOperand(0);
-    return TLI.makeLibCall(DAG, LC, N->getValueType(0), Op, isSigned,
-                           dl).first;
+    return TLI.makeLibCall(DAG, LC, N->getValueType(0), Op, isSigned, dl).first;
   } else if (NumOps == 2) {
----------------
spurious formatting change?

================
Comment at: lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp:170
@@ -164,1 +169,3 @@
+    LegalizeOp(SDValue(&*I, ResNo));
+  }
 
----------------
Not clear how it relates to the "FP with chain" stuff.

================
Comment at: lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:114
@@ -113,3 +113,2 @@
   case ISD::FMAXNAN:
-
   case ISD::FPOW:
----------------
spurious formatting change

================
Comment at: lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:2058
@@ -1997,2 +2057,3 @@
   case ISD::FREM:
+  case ISD::FREM_W_CHAIN:
   case ISD::SDIV:
----------------
In the rest of the patch, I think you grouped the *_W_CHAIN cases.

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAG.cpp:3928
@@ +3927,3 @@
+    case ISD::FREM_W_CHAIN:
+      return N1;     // fold op(undef, arg2) -> undef
+    case ISD::FADD_W_CHAIN:
----------------
I'm not convince you maintain correct FPenv semantics here.

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAG.cpp:3936
@@ +3935,3 @@
+
+  if (getTarget().Options.UnsafeFPMath && N2.getOpcode() == ISD::UNDEF)
+    return N2;     // fold op(arg1, undef) -> undef
----------------
Another case of mixing UnsafeFPMath with FPenv, I'm not sure about where this is going. Shouldn't we just drop the chain and turn every XXX_W_CHAIN into XXX when UnsafeFPMath is enabled?

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:2352
@@ +2351,3 @@
+  } else if (isa<ConstantExpr>(&I)) {
+    // Constant expressions don't store fast-math flags, so fill those related
+    // to floating-point access from global options.
----------------
This has changed recently :)


Repository:
  rL LLVM

http://reviews.llvm.org/D14079