This is an archive of the discontinued LLVM Phabricator instance.

[mips] Fix 64bit slt/sltu/nor with immediates
ClosedPublic

Authored by arichardson on Feb 24 2017, 3:17 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

sdardis edited edge metadata.Feb 24 2017, 7:47 AM

Looks mostly ok. See my inlined comment about handling sltu.

lib/Target/Mips/Mips64InstrInfo.td
723–734 ↗(On Diff #89638)

Rather than providing an alias which covers the imm64 case, instead provide an pseudo instruction like the old version of NORImm which takes which takes GPR64Opnds and imm64. Then unconditionally expand it like NORImm. and set the final opcode to the corresponding comparison. You will be able to provide a two operand alias then.

arichardson edited the summary of this revision. (Show Details)

Made the two argument version work

sdardis accepted this revision.Feb 27 2017, 3:16 AM

LGTM with the inline comments addressed. The logic error with the (outs) and (ins) appears to be doing the right thing, but should be corrected anyway.

Thanks for doing these patches.

lib/Target/Mips/Mips64InstrInfo.td
695 ↗(On Diff #89723)

This entire hunk should be under "Assembler Pseudo Instructions".

698–700 ↗(On Diff #89723)

Logic error: the GPR64Opnd:$rs should be with the outs.

Formatting. Keep the MipsAsmPseudoInst on the same line as the 'def'. When the line exceeds 80 chars, break it at the (ins ) construct, and align the (ins ..) with the outs above. E.g:

def SLTImm64 : MipsAsmPseudoInst<(outs GPR64Opnd:$rs),
                                 (ins GPR64Opnd:$rt, imm64:$imm),
                                 "slt\t$rs, $rt, $imm">, GPR_64;
703–705 ↗(On Diff #89723)

Logic error: the GPR64Opnd:$rs should be with the outs.

Formatting as well.

707 ↗(On Diff #89723)

imm64:$imm should be aligned under the GPR64Opnd:$rs.

lib/Target/Mips/MipsInstrInfo.td
2520 ↗(On Diff #89723)

Logic error: RO: $rs should be with the 'outs'.

This revision is now accepted and ready to land.Feb 27 2017, 3:16 AM
arichardson marked 6 inline comments as done.

Addressed inline comments

If this looks good now could you please commit for me?

lib/Target/Mips/MipsInstrInfo.td
2520 ↗(On Diff #89723)

That makes sense, I was a bit surprised when I copied it from the existing NORImm.

This revision was automatically updated to reflect the committed changes.
llvm/trunk/lib/Target/Mips/MipsInstrInfo.td