This is an archive of the discontinued LLVM Phabricator instance.

[mips] Fix floating point select machine verifier errors
ClosedPublic

Authored by smaksimovic on Jul 14 2017, 3:54 AM.

Details

Summary

Introduced FSELECT node necesary when lowering ISD::SELECT which
has i32, f64, f64 as its operands.
SEL_D instruction required that its output and first operand of
SELECT node, which it used, have matching types.
MTC1_D64 node introduced to aid FSELECT lowering.

This fixes machine verifier errors on following files:
CodeGen/Mips/llvm-ir/select-dbl.ll
CodeGen/Mips/llvm-ir/select-flt.ll
CodeGen/Mips/select.ll

Diff Detail

Event Timeline

smaksimovic created this revision.Jul 14 2017, 3:54 AM
sdardis accepted this revision.Jul 19 2017, 9:14 AM

LGTM with some (minor) nits addressed.

lib/Target/Mips/Mips32r6InstrInfo.td
581–585

Nit: Can you move this to the top of the file before the operand descriptions with the same section header as found in MipsInstrInfo.td? Also, add a section header for the operands.

I'd like to keep the file organised in a similar fashion to MipsInstrInfo.td.

lib/Target/Mips/MipsISelLowering.h
71

This move instruction requires it's own comment.

lib/Target/Mips/MipsInstrFPU.td
42–43

Nit, the textual order of the type of operands doesn't match the type declarations, i.e. this should be:

def SDT_MipsMTC1_D64 : SDTypeProfile<1, 1, [SDTCisVT<0, f64>,
                                            SDTCisVT<1, i32>]>;
lib/Target/Mips/MipsSEISelLowering.cpp
372

if(!Subtarget.hasMips32r6()) is sufficient here as hasMips64r6() implies hasMips32r6().

378

Nit: add a comment explaining that although MTC1_D64 takes an i32 and writes an f64, the upper 32 bits of the floating point register are undefined. This isn't an issue as sel.d which is produced from an FSELECT node only looks at bit 0.

This revision is now accepted and ready to land.Jul 19 2017, 9:14 AM
This revision was automatically updated to reflect the committed changes.