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

[Jay Foad via Phabricator via llvm-commits]

foad added a comment.

I think this looks good now. Are you happy with it @craig.topper @sepavloff ?



================
Comment at: llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:2258
+    // Convert the metadata argument to a constant integer
+    Metadata *MD = cast<MetadataAsValue>(CI.getOperand(1))->getMetadata();
+    Optional<RoundingMode> RoundMode =
----------------
Should probably use getArgOperand here, though it doesn't make any difference except for bounds checking the index that you pass in.


================
Comment at: llvm/lib/IR/Verifier.cpp:4750
+
+    Assert(dyn_cast<MDString>(MD),
+           ("invalid value for llvm.fptrunc.round metadata operand"
----------------
`isa` is a bit more natural than `dyn_cast` here.


================
Comment at: llvm/test/CodeGen/AMDGPU/fail.llvm.fptrunc.round.ll:1
+; RUN: not --crash llc -march=amdgcn -mcpu=gfx1030 -verify-machineinstrs -o /dev/null %s 2>&1 | FileCheck %s --ignore-case --check-prefix=FAIL
+; RUN: not --crash llc -global-isel -march=amdgcn -mcpu=gfx1030 -verify-machineinstrs -o /dev/null %s 2>&1 | FileCheck %s --ignore-case --check-prefix=FAIL
----------------
Does this test work in a Release build? If not, you can add a "REQUIRES: asserts" line just after the RUN lines.


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

https://reviews.llvm.org/D110579