Page MenuHomePhabricator

[AsmPrinter] Print FP constant in hexadecimal form instead
ClosedPublic

Authored by jsji on Jan 28 2020, 10:49 AM.

Details

Summary

Printing floating point number in decimal is inconvenient for humans.
Verbose asm output will print out floating point values in comments, it
helps.

But in lots of cases, users still need additional work to covert the
decimal back to hex or binary to check the bit patterns,
especially when there are small precision difference.

Hexadecimal form is one of the supported form in LLVM IR, and easier for
debugging.

This patch try to print all FP constant in hex form instead.

Diff Detail

Event Timeline

jsji created this revision.Jan 28 2020, 10:49 AM
jsji edited the summary of this revision. (Show Details)Jan 28 2020, 10:52 AM

Unit tests: pass. 62268 tests passed, 0 failed and 827 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

jsji added a comment.Feb 4 2020, 6:11 AM

Ping.. Any feedback and comments are appreciated. Thanks.

@craig.topper Any comments?

Seems reasonable to me. @RKSimon did you have a concern?

@craig.topper Any comments?

Seems reasonable to me. @RKSimon did you have a concern?

Sorry no, I was getting this confused with your old D37184 patch

My only question is whether we should have leading zeros to guarantee a specific width for a given type?

llvm/test/CodeGen/X86/pr40730.ll
23

Should all these be the full .quad width (i.e. leading zeros)?

jsji updated this revision to Diff 242708.Feb 5 2020, 11:21 AM

Thanks @RKSimon and @craig.topper .

Yes, good suggestion.
Updated to include leading zero paddings.

Unit tests: pass. 62268 tests passed, 0 failed and 827 were skipped.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

RKSimon added inline comments.Feb 5 2020, 1:05 PM
llvm/include/llvm/MC/MCExpr.h
134

unsigned SizeInBytes = 0;

jsji updated this revision to Diff 242745.Feb 5 2020, 1:54 PM

Rename Size to SizeInBytes.

jsji marked 2 inline comments as done.Feb 5 2020, 1:55 PM

Unit tests: pass. 62268 tests passed, 0 failed and 827 were skipped.

clang-tidy: pass.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

jsji added a comment.Feb 5 2020, 2:25 PM

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

I believe this is bug in pre-merge bot. I did run clang-format before updating.
And I double confirmed again, nothing needs change on my
$ git clang-format HEAD^
clang-format did not modify any files

RKSimon accepted this revision.Feb 6 2020, 6:53 AM

LGTM - thanks

This revision is now accepted and ready to land.Feb 6 2020, 6:53 AM
This revision was automatically updated to reflect the committed changes.