This is an archive of the discontinued LLVM Phabricator instance.

[fpcmp] Use non-floating point parsing by default.
ClosedPublic

Authored by Meinersbur on Jul 1 2022, 2:54 PM.

Details

Summary

Unless using the -a or -r options, do not parse floating point numbers in order to compare its numeric difference. Despite its name, fpcmp used used to compare every program output with its reference output even for programs that do not handle floating point and caused 0. and 0 to be always considered equal.

I the old behaviour is needed (as for the TSVC benchmark), floating point comparison with zero numeric tolerance can be enabled by explicitly setting -a 0 or -r 0.

Motivated by D128262.

Event Timeline

Meinersbur created this revision.Jul 1 2022, 2:54 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 1 2022, 2:54 PM
Herald added a subscriber: mgorny. · View Herald Transcript
Meinersbur requested review of this revision.Jul 1 2022, 2:54 PM
  • Advance pointer on match
MatzeB accepted this revision.Jul 18 2022, 9:43 AM

LGTM

BTW: I always wanted to switch the tools to use diff by default which is faster and produces more helpful output in case of differences; and only use fpcmp for the few benchmarks that need float tolerances. Anyway this change makes sense in the meantime.

MultiSource/Benchmarks/TSVC/CMakeLists.txt
2–6

I wonder if we shouldn't rather change the reference output. But fine with me if things aren't that easy to fix.

tools/fpcmp.c
385–386

Why didn't you also combine the last string with the others?

This revision is now accepted and ready to land.Jul 18 2022, 9:43 AM
  • Collapse fprintf calls
Meinersbur added inline comments.Jul 18 2022, 12:30 PM
MultiSource/Benchmarks/TSVC/CMakeLists.txt
2–6

Different printf implementations may implement floating point formatting differently. I only tested under Linux, I don't know what other libc implementations do and I don't think the test should fail on other platforms.

Generally, I am hesitant to change the reference output, which was hand-vetted and may have come from the benchmark itself. For SPEC benchmarks we cannot because the references come with the benchmark. D128261 is another case where the output is different between LLVM and GNU, but both seem to be correct.

tools/fpcmp.c
385–386

I think I left the last string unchanged at first and only later added "also". Changed.

This revision was automatically updated to reflect the committed changes.