[PATCH] D110579: [AMDGPU] Add a new intrinsic to control fp_trunc rounding mode

[Jay Foad via Phabricator via llvm-commits]

foad added a comment.

This is looking pretty good, just another round of minor comments.



================
Comment at: llvm/docs/LangRef.rst:23870
+<t_floating>` value to cast and a :ref:`floating-point <t_floating>` type
+to cast it to. The size of ``value`` must be larger than the size of ``ty2``.
+
----------------
This seems to be repeating the "must be larger" text from a few lines above.


================
Comment at: llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:2270
+
+    return MIB;
+  }
----------------
`return true`.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:6337
+    // Build the operand list.
+    SmallVector<SDValue, 4> Ops;
+
----------------
You don't really need a SmallVector here, since you know you have exactly two operands, you could just pass them as two separate arguments to the DAG.getNode call. But it's up to you.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:6341
+    // call
+    Metadata *MD = cast<MetadataAsValue>(I.getOperand(1))->getMetadata();
+    Optional<RoundingMode> RoundMode =
----------------
Shouldn't this be get**Arg**Operand(1)? How does it work?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:6345
+
+    TargetLowering::IntrinsicInfo Info;
+    const TargetLowering &TLI = DAG.getTargetLoweringInfo();
----------------
This isn't used.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:6346
+    TargetLowering::IntrinsicInfo Info;
+    const TargetLowering &TLI = DAG.getTargetLoweringInfo();
+
----------------
TLI is already available in this function.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:6359
+    SDNodeFlags Flags;
+    if (auto *FPMO = dyn_cast<FPMathOperator>(&I))
+      Flags.copyFMF(*FPMO);
----------------
All intrinsics returning floating point type are classed as FPMathOperators, so you can do this part unconditionally.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:6364
+    SDValue Result;
+    Result = DAG.getNode(ISD::FPTRUNC_ROUND, getCurSDLoc(), VT, Ops);
+    setValue(&I, Result);
----------------
Use sdl here too.


================
Comment at: llvm/lib/IR/Verifier.cpp:4748
+
+    Assert(MD != nullptr, "missing rounding mode argument", Call);
+
----------------
I am not an expert on verifying metadata but I think you probably need to check that it is an MDString here, otherwise the cast<MDString> below could fail? And really we ought to have some tests in test/Verifier/llvm.fptrunc.round.ll.


================
Comment at: llvm/lib/IR/Verifier.cpp:4752
+        convertStrToRoundingMode(cast<MDString>(MD)->getString());
+    Assert(RoundMode.hasValue() && RoundMode != RoundingMode::Dynamic,
+           "unsupported rounding mode argument", Call);
----------------
I am not sure but I think `RoundMode != RoundingMode::Dynamic` needs to be `RoundMode.getValue() != ...` (or `*RoundMode != ...`).


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:4662
+
+    // Get the rounding mode from the last operand
+    int RoundMode = cast<ConstantSDNode>(Op.getOperand(1))->getZExtValue();
----------------
Do you also need to check that the operand is MVT::f32? Maybe add a test where the operand is f64, and check that it fails to legalize?


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

https://reviews.llvm.org/D110579