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

[Matt Arsenault via llvm-commits]

arsenm added a comment.

Please drop the AMDGPU changes. I don't see a point in trying to support these at this time, and they can't be meaningfully tested now. While the hardware supports floating point exceptions, they require a lot of additional compiler work to implement beyond the core operations. A lot of SGPRs need to be reserved and initialized, the control registers set to enable them, somehow supporting trap handlers and probably other things.

Other than that, Legal is a bad default for these. These should use the new LibCall action by default set in TargetLoweringBase. There is also a hasFloatingPointExceptions() target hook already, perhaps something could error if these are used and not supported.


================
Comment at: include/llvm/CodeGen/ISDOpcodes.h:243
@@ -242,1 +242,3 @@
 
+    /// Simple inary floating point operators with side effects that have token
+    /// chains as their first operand.
----------------
Typo: inary

================
Comment at: lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:1393-1395
@@ +1392,5 @@
+            Node->getValueType(1) == MVT::Other) {
+          SmallVector<SDValue, 2> ResultVals;
+          ResultVals.push_back(Res.getValue(0));
+          ResultVals.push_back(DAG.getEntryNode());
+          ReplaceNode(Node, ResultVals.data());
----------------
You can use a statically sized C array for this

================
Comment at: lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:4344
@@ +4343,3 @@
+    Results.push_back(DAG.getNode(ISD::FP_ROUND, dl, OVT,
+                                  Tmp3, DAG.getIntPtrConstant(0, dl)));
+    Results.push_back(Tmp3.getValue(1));
----------------
Using the integer size for address space 0 seems like a poor choice. Do targets actually care about this type? Why isn't it an i1 target constant?

================
Comment at: lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp:202-206
@@ +201,7 @@
+  for (const SDValue &Op : Node->op_values()) {
+    if ((Op.getOpcode() == ISD::FADD_W_CHAIN ||
+         Op.getOpcode() == ISD::FSUB_W_CHAIN ||
+         Op.getOpcode() == ISD::FDIV_W_CHAIN ||
+         Op.getOpcode() == ISD::FREM_W_CHAIN ||
+         Op.getOpcode() == ISD::FMUL_W_CHAIN) &&
+        Op.getValueType() == MVT::Other)
----------------
A new utility function for isFPOpWithChain would be useful.

================
Comment at: lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp:208-209
@@ +207,4 @@
+        Op.getValueType() == MVT::Other)
+      // Do not attempt to legalize chain.
+      Ops.push_back(Op.getOperand(0));
+    else
----------------
Why is this Op.getOperand(0) instead of just Op? I would also move the chain handling out of the loop and handle separately or have a separate w/chain and wo/chain loop

================
Comment at: lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:2182-2186
@@ +2181,7 @@
+  if (NumElts == 1) {
+    SDValue Chain;
+    SDValue Ret = DAG.UnrollVectorOp(N, WidenVT.getVectorNumElements(), &Chain);
+    if (Chain)
+      AnalyzeNewNode(Chain.getNode());
+    return Ret;
+  }
----------------
A Chain out arguments looks weird here. Why can't UnrollVectorOp's result have the chain result?

================
Comment at: lib/CodeGen/SelectionDAG/TargetLowering.cpp:92
@@ -91,1 +91,3 @@
                             bool isReturnValueUsed) const {
+  bool HasChain = (Ops.size() != 0 && Ops[0].getValueType() == MVT::Other);
+
----------------
Ops.empty()

================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:1309-1312
@@ -1300,2 +1308,6 @@
 
+  if (HasChain)
+    return DAG.getNode(ISD::FMUL_W_CHAIN, SL,
+                       DAG.getVTList(MVT::f32, MVT::Other), Op.getOperand(0),
+                       r3, Mul);
   return DAG.getNode(ISD::FMUL, SL, MVT::f32, r3, Mul);
----------------
Why would only the last fmul need to be FMUL_W_CHAIN?

================
Comment at: lib/Target/AMDGPU/SIInstructions.td:1450-1454
@@ -1449,2 +1449,7 @@
 
+let isCodeGenOnly = 1, hasSideEffects = 1 in
+  defm V_ADD_F32_wchain : VOP2Inst <vop2<0x3, 0x1>, "v_add_f32_wchain",
+    VOP_F32_F32_F32, faddwchain
+  >;
+
 defm V_SUB_F32 : VOP2Inst <vop2<0x4, 0x2>, "v_sub_f32", VOP_F32_F32_F32, fsub>;
----------------
The instruction asm string should not include the _wchain. A better name I think for this would be V_ADD_F32_FPE or something else that doesn't mention chains.


Repository:
  rL LLVM

http://reviews.llvm.org/D14079