Page MenuHomePhabricator

[ConstantFolding] Fix GetConstantFoldFPValue to avoid cast overflow.
ClosedPublic

Authored by bixia on Mar 18 2019, 10:31 AM.

Details

Summary

In C++, the behavior of casting a double value that is beyond the range
of a single precision floating-point to a float value is undefined. This
change replaces such a cast with APFloat::convert to convert the value,
which is consistent with how we convert a double value to a half value.

Diff Detail

Event Timeline

bixia created this revision.Mar 18 2019, 10:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 18 2019, 10:31 AM
sanjoy requested changes to this revision.Mar 18 2019, 1:23 PM

This change needs a test, as Roman said.

This revision now requires changes to proceed.Mar 18 2019, 1:23 PM
bixia updated this revision to Diff 191326.Mar 19 2019, 9:12 AM

Add a test case.

bixia updated this revision to Diff 191328.Mar 19 2019, 9:13 AM

Update commit message.

Harbormaster completed remote builds in B29355: Diff 191328.
lebedev.ri added inline comments.Mar 19 2019, 9:18 AM
test/Transforms/ConstProp/calls.ll
199

Bad test. It already is optimized.
https://godbolt.org/z/Y2DAxa

bixia added a comment.Mar 19 2019, 1:38 PM

The problem is only reported by ubsan (undefined behavior sanitizer). My test case produces a value of 1.27e+80, which is a value between max(float) and max(double). Without ubsan, llvm opt (compiled with clang for x86) produces the correct result silently. However, since (float)the-double-value has undefined behavior by c++ language, there is no guarantee that the llvm opt produced by any compiler for any platform produces the same result. Do you have suggestion on what is a better test case?

The problem is only reported by ubsan (undefined behavior sanitizer). My test case produces a value of 1.27e+80, which is a value between max(float) and max(double). Without ubsan, llvm opt (compiled with clang for x86) produces the correct result silently. However, since (float)the-double-value has undefined behavior by c++ language, there is no guarantee that the llvm opt produced by any compiler for any platform produces the same result. Do you have suggestion on what is a better test case?

That test doesn't test any of what you have just said.
It only tests that there is no call in the output.
At the very least you probably want to actually test the output, not lack of unwanted output.

bixia updated this revision to Diff 191419.Mar 19 2019, 5:09 PM

Modify test case to check the return value.

sanjoy accepted this revision.Mar 19 2019, 6:27 PM

Lgtm

This revision is now accepted and ready to land.Mar 19 2019, 6:27 PM
lebedev.ri added inline comments.Mar 20 2019, 12:13 AM
test/Transforms/ConstProp/calls.ll
196

Please add a comment as to what this testing

sanjoy added inline comments.Mar 20 2019, 12:19 AM
test/Transforms/ConstProp/calls.ll
196

Fwiw I think the f32_overflow suffix makes it clear what it is testing and is consistent with the rest of the file.

This revision was automatically updated to reflect the committed changes.