This is an archive of the discontinued LLVM Phabricator instance.

[libc][NFC] Make tuning macros start with LIBC_COPT_
ClosedPublic

Authored by gchatelet on Feb 13 2023, 7:16 AM.

Details

Summary

Rename preprocessor definitions that control tuning of llvm libc.

Diff Detail

Event Timeline

gchatelet created this revision.Feb 13 2023, 7:16 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptFeb 13 2023, 7:16 AM
gchatelet requested review of this revision.Feb 13 2023, 7:16 AM
gchatelet updated this revision to Diff 496974.Feb 13 2023, 7:25 AM
  • Keep CMake option as is, only change preprocessor

@michaelrj it seems that the following definitions are currently not exposed to the build system.
LIBC_COPT_PRINTF_CONV_ATLAS
LIBC_COPT_PRINTF_DISABLE_FLOAT
LIBC_COPT_PRINTF_DISABLE_INDEX_MODE
LIBC_COPT_PRINTF_DISABLE_WRITE_INT
LIBC_COPT_PRINTF_INDEX_ARR_LEN
LIBC_COPT_PRINTF_CONV_ATLAS
LIBC_COPT_SCANF_DISABLE_FLOAT
LIBC_COPT_SCANF_DISABLE_INDEX_MODE
LIBC_COPT_DISABLE_CLINGER_FAST_PATH
LIBC_COPT_DISABLE_EISEL_LEMIRE
LIBC_COPT_DISABLE_SIMPLE_DECIMAL_CONVERSION

Also for the last three ones we should scope it, any suggestions ? How about LIBC_COPT_STRTOFLOAT_DISABLE_ ?

gchatelet added inline comments.Feb 13 2023, 7:40 AM
libc/test/src/math/RoundToIntegerTest.h
89

@lntue I'm unsure what this is about so I didn't rename it. Is it supposed to be exposed at some point?
There is a CMake option here but it is not sufficient to pass the option down to the C++ code.

lntue added inline comments.Feb 13 2023, 10:20 AM
libc/test/src/math/RoundToIntegerTest.h
89

I think we planned to have a section on the documentations about all the implementation-defined cases where we are different from glibc and maybe provide these flags for users.

The flags for my pieces don't have cmake options because I didn't know how we were going to organize them at first.

For printf:
Disable float, disable index mode, and disable write int are all independent flags. They can have simple options and I think they should default to off, off, and on respectively (so that we only disable write int).
Index arr len is takes an int, but it's already got a default in printf_config.h so its cmake options should probably default to off.
Conv atlas is intended to let users completely replace the conversion logic, since it defines where the header file that includes all the headers that define the converters is. I'd say it probably shouldn't have a cmake option at all, since the cmake targets that define the header libraries for the conversions would also need to be changed. Basically, it's for people who are using their own build system and want to use their own conversions.

For scanf:
Disable float and index mode are the same as in printf.

For strtofloat:
Scoping those options seems like a good idea. They can similarly have simple options, although it might be good to have a warning pop up when disabling simple decimal conversion since that means there's no guarantee of correctness.

gchatelet marked an inline comment as done.Feb 14 2023, 4:07 AM

@phosek @mcgrathr this patch would also rename LLVM_LIBC_TEST_USE_FUCHSIA to LIBC_COPT_TEST_USE_FUCHSIA to conform to our guidelines.
Let me know if that's an issue for you.

libc/test/src/math/RoundToIntegerTest.h
89

So IIUC this is a customization point and it should be renamed.
I'll do so, let me know if you think otherwise.

gchatelet updated this revision to Diff 497270.Feb 14 2023, 4:19 AM
gchatelet marked an inline comment as done.
  • rename LLVM_LIBC_IMPLEMENTATION_DEFINED_TEST_BEHAVIOR as well
  • Scope str_to_float macros and add a warning

The flags for my pieces don't have cmake options because I didn't know how we were going to organize them at first.

For printf:
Disable float, disable index mode, and disable write int are all independent flags. They can have simple options and I think they should default to off, off, and on respectively (so that we only disable write int).
Index arr len is takes an int, but it's already got a default in printf_config.h so its cmake options should probably default to off.
Conv atlas is intended to let users completely replace the conversion logic, since it defines where the header file that includes all the headers that define the converters is. I'd say it probably shouldn't have a cmake option at all, since the cmake targets that define the header libraries for the conversions would also need to be changed. Basically, it's for people who are using their own build system and want to use their own conversions.

For scanf:
Disable float and index mode are the same as in printf.

For strtofloat:
Scoping those options seems like a good idea. They can similarly have simple options, although it might be good to have a warning pop up when disabling simple decimal conversion since that means there's no guarantee of correctness.

Thx for the detailed answer!
I've scoped the strtofloat macros. I'll expose the options in a subsequent patch.

LGTM for my options. Thank you for doing this!

lntue accepted this revision.Feb 14 2023, 10:41 AM
This revision is now accepted and ready to land.Feb 14 2023, 10:41 AM
This revision was automatically updated to reflect the committed changes.