yaxunl marked 4 inline comments as done.
yaxunl added inline comments.
Herald added subscribers: jansvoboda11, dexonsmith, dang.


================
Comment at: clang/include/clang/Basic/CodeGenOptions.def:399
 
+/// Whether to emit IEEE754-2008 NaN compliant instructions if available 
(AMDGPU Only)
+CODEGENOPT(EmitIEEENaNCompliantInsts, 1, 1)
----------------
arsenm wrote:
> Description is misleading. Better description would be the first line from 
> the manual, 
> "Floating point opcodes that support exception flag gathering quiet and 
> propagate sig- naling NaN inputs per IEEE 754-2008"
will do


================
Comment at: clang/include/clang/Driver/Options.td:2406
+def mamdgpu_ieee : Flag<["-"], "mamdgpu-ieee">, Flags<[CC1Option]>,
+  Group<m_Group>, HelpText<"Enable IEEE754-2008 NaN compliance in supported 
AMDGPU instructions">;
+def mno_amdgpu_ieee : Flag<["-"], "mno-amdgpu-ieee">, Flags<[CC1Option]>,
----------------
arsenm wrote:
> Ditto
will do


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:1434
+
+  Opts.EmitIEEENaNCompliantInsts =
+      Args.hasFlag(options::OPT_mamdgpu_ieee, options::OPT_mno_amdgpu_ieee,
----------------
arsenm wrote:
> Add a comment explaining why to turn it off? Also should note this is only 
> really concerns signaling nans
will do


================
Comment at: clang/test/CodeGenOpenCL/amdgpu-ieee.cl:20
+}
+
+// ON-NOT: attributes [[ATTRS]] = {{.*}} "amdgpu-ieee"
----------------
arsenm wrote:
> arsenm wrote:
> > Should also test a non-kernel function
> I think we should also have some ISA check run lines that show the final 
> result for minnum/maxnum lowering, as well as if output modifiers are used
will do


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

https://reviews.llvm.org/D77013

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to