Page MenuHomePhabricator

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

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

Details

Summary

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

Repository
rL LLVM

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
test/CodeGen/Mips/f32-to-i64-single-float.ll
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
test/CodeGen/Mips/f32-to-i64-single-float.ll
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
test/CodeGen/Mips/f32-to-i64-single-float.ll
1 ↗(On Diff #164493)

You can use update_llc_checks.py 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 update_llc_checks.py 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.