This is an archive of the discontinued LLVM Phabricator instance.

[libc] Add rounding mode support for MPFR testing macros.
ClosedPublic

Authored by lntue on Jan 6 2022, 4:14 PM.

Details

Summary

Add an extra argument for rounding mode to EXPECT_MPFR_MATCH and ASSERT_MPFR_MATCH macros.

Diff Detail

Event Timeline

lntue created this revision.Jan 6 2022, 4:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 6 2022, 4:14 PM
lntue requested review of this revision.Jan 6 2022, 4:14 PM
This revision is now accepted and ready to land.Jan 6 2022, 5:08 PM
sivachandra added inline comments.Jan 7 2022, 1:01 PM
libc/utils/MPFRWrapper/MPFRUtils.h
306–319

I am not sure if this MISSING_ARGS macro is any useful. For, if any error occurs, it will print an error message but tell us nothing about what is actually wrong. I would think a better approach would be to let the RoundingMode arg to get_mpfr_matcher take a default value of RoundingMode::Nearest. That way, you can also simplify the macros below to some thing like this:

#define EXPECT_MPFR_MATCH(op, input, match_value, tolerance, ...) \
  EXPECT_THAT(match_value,                                        \
              __llvm_libc::testing::mpfr::get_mpfr_matcher<op>(   \
                  input, match_value, tolerance                   \
                  __VA_OPT__(,) __VA_ARGS__))

This way, if there is any error in the args, we will actually get a proper compiler error message. If using __VA_OPT__ is a concern, we can use -std=c++20 for compiling MPFRWrapper.

lntue updated this revision to Diff 398958.Jan 11 2022, 8:20 AM

Change MISSING_ARGS to GET_MPFR_DUMMY_ARG.

lntue added inline comments.Jan 11 2022, 8:57 AM
libc/utils/MPFRWrapper/MPFRUtils.h
306–319

I use MISSING_ARGS as a simple way to avoid gnu-zero-variadic-macro-arguments warnings. It is simpler and does not require extra compiler flags as VA_OPT option. I changed it to GET_MPFR_DUMMY_ARG and added comments to make the intention clearer.

sivachandra added inline comments.Jan 12 2022, 12:10 AM
libc/utils/MPFRWrapper/MPFRUtils.h
306–319

Ah! So, why should GET_MPFR_MACRO be a variadic macro at all?

310

Add double-underscore prefix to internal macro names.

lntue added inline comments.Jan 12 2022, 4:05 AM
libc/utils/MPFRWrapper/MPFRUtils.h
306–319

To support and not litter all current usages of EXPECT_MPFR_MATCH with default rounding mode as the 5th argument, it is expanded to either EXPECT_MPFR_MATCH_ROUNDING with 5 arguments, or EXPECT_MPFR_MATCH_DEFAULT with 4 arguments by GET_MPFR_MACRO.

If 5 arguments are provided to EXPECT_MPFR_MATCH, GET_MPFR_MACRO will get 6 arguments (not including GET_MPFR_DUMMY_ARG), and if 4 arguments are provided to EXPECT_MPFR_MATCH, GET_MPFR_MACRO will only get 5 arguments (not including GET_MPFR_DUMMY_ARG), and hence it needs to be variadic.

It is the 4 argument case that variadic GET_MPFR_MACRO got complained by -Wgnu-zero-variadic-macro-arguments without GET_MPFR_DUMMY_ARG.

lntue updated this revision to Diff 399391.Jan 12 2022, 11:20 AM

[libc] Add rounding mode support for MPFR testing macros.

sivachandra added inline comments.Jan 12 2022, 11:28 AM
libc/utils/MPFRWrapper/MPFRUtils.h
306–319

So, the way you have set it up, it can already support 4 or 5 args. But, it need not be a variadic macro. FWIW, I patched your change and removed the ... and also the last arg to the macro on line 309 and line 325. It goes through fine.

lntue updated this revision to Diff 399543.Jan 12 2022, 9:16 PM

Add simple tests for sqrt with different rounding modes.
Make all the MPFR util functions accept both precision and rounding mode.

lntue added inline comments.Jan 12 2022, 9:18 PM
libc/utils/MPFRWrapper/MPFRUtils.h
306–319

Because there were no tests using the updated macros with rounding parameters. I've added simple tests for sqrt with different rounding modes. Now the build would fail if you remove ... from GET_MPFR_MACRO.

sivachandra accepted this revision.Jan 12 2022, 10:01 PM
sivachandra added inline comments.
libc/utils/MPFRWrapper/MPFRUtils.cpp
149–152

Nit: you should probably use XType here and below instead of float etc.

libc/utils/MPFRWrapper/MPFRUtils.h
306–319

Thanks! That helps me see it better. I still prefer to keep it simple really. The new macro can be called ASSERT_MPFR_MATCH_WITH_ROUNDING and make it explicit. I agree it is verbose, but keeps everything really simple. For, the error message I see now says that there are incorrect number of args somewhere, but does not tell me where exactly. I will leave it up to you do decide.

310

This is still not addressed.

sivachandra added inline comments.Jan 12 2022, 11:25 PM
libc/test/src/math/SqrtTest.h
69

I am assuming this is just a demo and that you will remove it before submitting.

lntue updated this revision to Diff 399710.Jan 13 2022, 10:05 AM
lntue marked 2 inline comments as done.

[libc] Add rounding mode support for MPFR testing macros.

lntue updated this revision to Diff 399717.Jan 13 2022, 10:22 AM
lntue marked an inline comment as done.

[libc] Add rounding mode support for MPFR testing macros.

lntue marked an inline comment as done.Jan 13 2022, 10:24 AM
This revision was automatically updated to reflect the committed changes.