[PATCH] D110579: [AMDGPU] Add two new intrinsics to control fp_trunc rounding mode

[Craig Topper via Phabricator via llvm-commits]

craig.topper added a comment.

I think I'd like to see a target independent ISD opcode added for this. We don't use ISD::INTRINSIC_WO_CHAIN for target independent intrinsics.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:6356
+    const Value *Arg = I.getArgOperand(0);
+    if (!I.paramHasAttr(0, Attribute::ImmArg))
+      Ops.push_back(getValue(Arg));
----------------
This is always false. And if it isn't you would only push one argument.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:6361
+    Ops.push_back(
+        DAG.getTargetConstant((int)RoundMode.getValue(), sdl, MVT::i8));
+
----------------
I'd probably just use TLI.getPointerTy instead of MVT::i8.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:6377
+
+    if (VectorType *PTy = dyn_cast<VectorType>(I.getType())) {
+      EVT VT = TLI.getValueType(DAG.getDataLayout(), PTy);
----------------
I know this was taken from visitTargetIntrinsic, but I don't really understand it.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:6381
+    } else
+      Result = lowerRangeToAssertZExt(DAG, I, Result);
+
----------------
I don't think this is needed. AssertZExt isn't useful for an FP result.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:6383
+
+    MaybeAlign Alignment = I.getRetAlign();
+    if (!Alignment)
----------------
I don't think this alignment stuff is needed. The return type is FP so it probably doesn't do anything.


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

https://reviews.llvm.org/D110579