Page MenuHomePhabricator

[MIPS] Fix illegal type assert in single-float mode

Authored by ZirconiumX on Sep 7 2018, 12:42 PM.



An fp_to_sint node would be incorrectly lowered to a TruncIntFP node in single-float mode. This would trigger an "Unexpected illegal type!" assert.

Many thanks to Tim Northover for helping me through this.

Diff Detail


Event Timeline

ZirconiumX created this revision.Sep 7 2018, 12:42 PM
ZirconiumX retitled this revision from [MIPS to [MIPS] Fix illegal type assert in single-float mode.Sep 7 2018, 12:46 PM
ZirconiumX edited the summary of this revision. (Show Details)
ZirconiumX set the repository for this revision to rL LLVM.
atanasyan added inline comments.Sep 9 2018, 2:08 PM
1 ↗(On Diff #164493)

It looks like you miss any CHECK statements. I understand that without the fix this test case triggers assert, but it's better to express expected assembler code and check them.

ZirconiumX added inline comments.Sep 9 2018, 2:14 PM
1 ↗(On Diff #164493)

The expected assembly code is...complicated, and while I've been dipping my toes into MIPS because of the PS2, even on -O0, LLVM inlines the conversion rather than producing something convenient to check like a library call. I'm not sure what exactly to check for, because my MIPS assembly knowledge isn't perfect.

sdardis added inline comments.Sep 10 2018, 10:37 AM
1 ↗(On Diff #164493)

You can use to generate the expected / current assembly result rather than hand-crafting CHECK lines. You'll need to change the -march=mips64el to -mtriple=mips64el-mti-linux-gnu .

3 ↗(On Diff #164493)

Also, remove the '#0' here and a add a comment describing the test case (a line or two is fine).

I've ran on my testcase, and added a brief description of the test as requested.

ZirconiumX marked 4 inline comments as done.Sep 11 2018, 5:53 AM

These should be fixed in the new diff.

atanasyan accepted this revision.Sep 11 2018, 5:56 AM

LGTM. Thanks. Do you have commit access?

This revision is now accepted and ready to land.Sep 11 2018, 5:56 AM

As this is my first patch, I do not, no.

This revision was automatically updated to reflect the committed changes.