[PATCH] D141863: [llvm][APFloat] Add NaN-in-negative-zero formats by AMD and GraphCore

[Reed Wanderman-Milne via Phabricator via llvm-commits]

reedwm added a comment.

Just some more minor nits about comments.



================
Comment at: llvm/lib/Support/APFloat.cpp:64-65
+  // Float8E5M2FNUZ, and Float8E4M3FNUZ). There is no representation for Inf,
+  // and operations that would ordinarily produce Inf produce NaN instead. There
+  // is a single NaN representation, the details of which are determined by the
+  // `fltNanEncoding` enum. We treat all NaNs as quiet, as the available
----------------
fltNanEncoding::AllOnes technically has two NaN representations: a NaN with the sign bit set and a NaN with the sign bit unset. I would change this setence to "The details of the NaN representation(s) are determined by the `fltNanEncoding` enum


================
Comment at: llvm/lib/Support/APFloat.cpp:81
+  // Represents the behavior in the Float8E4M3 floating point type where NaN is
+  // represented by hhaving the exponent and mantissa set to all 1s.
+  // This behavior matches the FP8 E4M3 type described in
----------------
typo: hhaving -> having


================
Comment at: llvm/lib/Support/APFloat.cpp:861
+    // Finite-only types do not distinguish signalling and quiet NaN, so
+    // make them all signallling.
     SNaN = false;
----------------
typo: signallling -> signalling


================
Comment at: llvm/unittests/ADT/APFloatTest.cpp:5377
+
+  // 2. NextUp of smalest negative denormal is +0
+  test = APFloat::getSmallest(APFloat::Float8E5M2FNUZ(), true);
----------------
Typo: smalest -> smallest


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141863