This is an archive of the discontinued LLVM Phabricator instance.

[ConstantFolding] Fold calls to FP remainder function
ClosedPublic

Authored by ekatz on Nov 3 2019, 1:06 PM.

Details

Summary

With the fixed implementation of the "remainder" operation, we can now add support to folding calls to it.

Diff Detail

Event Timeline

ekatz created this revision.Nov 3 2019, 1:06 PM
arsenm added a subscriber: arsenm.Nov 3 2019, 7:35 PM

Needs test

ekatz updated this revision to Diff 227737.Nov 4 2019, 10:36 AM

Added a test (copied from the one for "fmod").
Added handling of "remainder" to TargetLibraryInfo.cpp.

arsenm added inline comments.Nov 4 2019, 11:09 AM
llvm/lib/Analysis/ConstantFolding.cpp
2103

Backwards condition

llvm/test/Analysis/ConstantFolding/math-2.ll
31

Should have tests stressing all the handled types , and preferably a few for the special cases , like snan

ekatz marked an inline comment as done.Nov 4 2019, 11:21 AM
ekatz added inline comments.
llvm/lib/Analysis/ConstantFolding.cpp
2103

Wanted to be consistent with the coding style on that particular file. See line 2095.
If I change this, should I also change it for "mod()" as well?

arsenm added inline comments.Nov 4 2019, 11:31 AM
llvm/lib/Analysis/ConstantFolding.cpp
2103

I would say change this, and leave the unrelated part alone

ekatz marked 2 inline comments as done.Nov 4 2019, 7:58 PM
ekatz added inline comments.
llvm/test/Analysis/ConstantFolding/math-2.ll
31

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.

ekatz updated this revision to Diff 227812.Nov 4 2019, 8:00 PM

Added a test case for double.

arsenm added inline comments.Nov 4 2019, 10:05 PM
llvm/test/Analysis/ConstantFolding/math-2.ll
31

The code specifically checks for opStatus though, so the point isn't necessarily then folding result

ekatz marked an inline comment as done.Nov 5 2019, 10:48 AM
ekatz added inline comments.
llvm/test/Analysis/ConstantFolding/math-2.ll
31

The APFloat unit test is also testing various combination of operations' results.

arsenm added inline comments.Nov 19 2019, 4:24 AM
llvm/test/Analysis/ConstantFolding/math-2.ll
31

But not in this usage context. The behavior here is changing based on it, so the point isn't testing what the opStatus is

37

also one for remainderl

ekatz marked an inline comment as done.Nov 19 2019, 9:32 AM
ekatz added inline comments.
llvm/test/Analysis/ConstantFolding/math-2.ll
37

remainderl is not folded. Just wanted to be consistent with other math operations (like fmodl). I am not sure why they are not folded, but it is consistent with other compilers like icc and gcc).

ekatz marked an inline comment as done.Nov 19 2019, 10:32 AM
ekatz added inline comments.
llvm/test/Analysis/ConstantFolding/math-2.ll
31

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).

hfinkel accepted this revision.Dec 13 2019, 5:39 PM
hfinkel added a subscriber: hfinkel.
hfinkel added inline comments.
llvm/test/Analysis/ConstantFolding/math-2.ll
31

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.

37

Yeah, this is indeed consistent with the other similar functions. I'm not sure why either, and we probably should do this (at least on systems where we accurately model the semantics of the long double type).

This revision is now accepted and ready to land.Dec 13 2019, 5:39 PM
ekatz marked an inline comment as done.Feb 12 2020, 3:17 AM
ekatz added inline comments.
llvm/test/Analysis/ConstantFolding/math-2.ll
31

Sounds good. I added another test for success in folding, and another for a failure.

This revision was automatically updated to reflect the committed changes.