[PATCH] D69777: [ConstantFolding] Fold calls to FP remainder function

[Ehud Katz via Phabricator via llvm-commits]

ekatz marked an inline comment as done.
ekatz added inline comments.


================
Comment at: llvm/test/Analysis/ConstantFolding/math-2.ll:31
+}
+
 declare double @pow(double, double)
----------------
hfinkel wrote:
> ekatz wrote:
> > arsenm wrote:
> > > ekatz wrote:
> > > > arsenm wrote:
> > > > > ekatz wrote:
> > > > > > arsenm wrote:
> > > > > > > Should have tests stressing all the handled types , and preferably a few for the special cases , like snan 
> > > > > > I agree that it should be tested for `double` as well, but the test cases are not important, as we test them under the APFloat unit-test. Otherwise, we will just have duplicates in the implementation code and the using code.
> > > > > The code specifically checks for opStatus though, so the point isn't necessarily then folding result
> > > > The APFloat unit test is also testing various combination of operations' results.
> > > But not in this usage context. The behavior here is changing based on it, so the point isn't testing what the opStatus is
> > Right, but wouldn't adding a bunch of other value combinations just test the result of APFloat? For which we already have the unit test. It doesn't only test the status, but also the actual result (bitwise).
> You don't need to add a large number of other tests, but you should have a test for something that shouldn't fold (V.remainder returns something other than opOK).
> 
> Otherwise, this LGTM.
Sounds good. I added another test for success in folding, and another for a failure.


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

https://reviews.llvm.org/D69777