This is an archive of the discontinued LLVM Phabricator instance.

Fix PR40644: miscompile indexed FP constant store
ClosedPublic

Authored by thopre on Oct 3 2019, 1:52 PM.

Details

Summary

Functions replaceStoreOfFPConstant() and OptimizeFloatStore() both
replace store of float by a store of an integer unconditionally. However
this generates wrong code when the store that is replaced is an indexed
or truncating store. This commit solves this issue by adding an early
return in these functions when the store being considered is not a
normal store.

Bug was only observed on out of tree targets, hence the lack of testcase
in this commit.

Diff Detail

Event Timeline

thopre created this revision.Oct 3 2019, 1:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 3 2019, 1:52 PM

Yep, that's what I had in mind. Do you have regression tests in the GC backend for these?

thopre added a comment.Oct 3 2019, 2:37 PM

Yep, that's what I had in mind. Do you have regression tests in the GC backend for these?

If I read correctly the commit that added the same logic in the backend, yes.

Tests missing.

Tests missing.

We've met this bug with an out of tree target. It requires a target with truncating or indexed constant float store which Eli Friedman was of the opinion is not the case of any of the in tree target. Still the change is small and obvious enough to be accepted. Both of these functions proceed with doing a regular integer store regardless of the initial store.

thopre added a comment.Nov 4 2019, 4:00 AM

Tests missing.

We've met this bug with an out of tree target. It requires a target with truncating or indexed constant float store which Eli Friedman was of the opinion is not the case of any of the in tree target. Still the change is small and obvious enough to be accepted. Both of these functions proceed with doing a regular integer store regardless of the initial store.

Ping?

efriedma accepted this revision.Nov 4 2019, 11:55 AM

Please add a note to the commit message about why there is no test.

LGTM

This revision is now accepted and ready to land.Nov 4 2019, 11:55 AM
thopre edited the summary of this revision. (Show Details)Nov 5 2019, 1:28 AM
This revision was automatically updated to reflect the committed changes.