[PATCH] D15385: [Support] Add saturating multiply-add support function

[Sean Silva via llvm-commits]

silvas added a comment.

Generally in LLVM for this kind of patch, we would add a couple motivating uses as well instead of as a separate patch. That helps show the context where it was intended to be used and that adding the helper is, in fact, a net win.


================
Comment at: include/llvm/Support/MathExtras.h:724
@@ +723,3 @@
+/// ResultOverflowed indicates if the result is larger than the maximum
+/// representable value of type T.
+template <typename T>
----------------
I think it's worth adding a note indicating that because all these values are unsigned, there is no distinction between a "fused" operation and non-fused from the perspective of whether saturation occurred.

================
Comment at: unittests/Support/MathExtrasTest.cpp:314
@@ +313,3 @@
+  // Test basic multiply-add.
+  EXPECT_EQ(T(16), SaturatingMultiplyAdd(T(2), T(3), T(10)));
+  EXPECT_EQ(T(16), SaturatingMultiplyAdd(T(2), T(3), T(10), &ResultOverflowed));
----------------
I don't think we should bother to repeatedly check the case without the `ResultOverflowed` (except maybe one case to make sure that it compiles (i.e. that the default argument does in fact have a default)).


http://reviews.llvm.org/D15385