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

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

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

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

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

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.