This is an archive of the discontinued LLVM Phabricator instance.

[mips][microMIPS] Implement SLT, SLTI, SLTIU, SLTU microMIPS32r6 instructions
ClosedPublic

Authored by hvarga on May 4 2016, 1:28 AM.

Details

Summary

This patch implements microMIPS32r6 SLT, SLTI, SLTIU and SLTU instructions.

There was a problem with the previous implementation of this patch (D19354) and because of that commit rL267137 was reverted.
After committing of the previous patch, test-suite failed with error message in the form of:

fatal error: error in backend: Cannot select: t21: i32 = setcc t2, Constant:i32<6>, setgt:ch

There was a problem with selecting SLT instruction in LLVM backend.

For that reason, it is decided to revert commit rL267137 and make this patch which besides implementation of instructions and standard regression tests also includes one additional CodeGen test (slt.ll).

Diff Detail

Repository
rL LLVM

Event Timeline

hvarga updated this revision to Diff 56103.May 4 2016, 1:28 AM
hvarga retitled this revision from to [mips][microMIPS] Implement SLT, SLTI, SLTIU, SLTU microMIPS32r6 instructions.
hvarga updated this object.
hvarga added subscribers: dsanders, sdardis, petarj, llvm-commits.
sdardis requested changes to this revision.May 5 2016, 5:07 AM
sdardis edited edge metadata.

Some nits. Can you extend the rest of the set*.ll tests?

lib/Target/Mips/MicroMipsInstrInfo.td
1009 ↗(On Diff #56103)

The SetgeImmPats patterns can be included for microMIPS if you extend that parameter list to include XOR_MM like the SeqeqPats patterns.

1047 ↗(On Diff #56103)

Also include the "sltu $rs, $rt, $imm" alias.

lib/Target/Mips/MipsInstrInfo.td
2189–2191 ↗(On Diff #56103)

This alias should be included in [NotinMicroMips].

2576 ↗(On Diff #56103)

See my above comment on extending SetgeImmPats.

test/CodeGen/Mips/slt.ll
4 ↗(On Diff #56103)

Use a CHECK-LABEL: to match the function here, and write a CHECK line that matches slt taking $4 as one of it operands as its an argument register or just "CHECK: slt".

9 ↗(On Diff #56103)

Same here.

This revision now requires changes to proceed.May 5 2016, 5:07 AM
hvarga updated this revision to Diff 62472.Jul 1 2016, 12:33 AM
hvarga edited edge metadata.
hvarga added a subscriber: mamidzic.

Added MipsPats for conditional branches in MicroMips.
Added predicate NotInMicroMips for existing Mips implementations.
Added cases for BNE_MM and BEQ_MM in getAnalyzableBrOpc, getEquivalentCompactForm and getOppositeBranchOpc.
Added tests for microMips in set*.ll files.

This update has been contributed by @mamidzic.

sdardis requested changes to this revision.Jul 1 2016, 6:49 AM
sdardis edited edge metadata.

Two major points: The multiclass patterns you've copied can instead be extended. LUi is also covered in a NotInMicroMips predicate. The rest is just formatting nits over the assembly. More details inline.

Thanks.

lib/Target/Mips/MicroMipsInstrInfo.td
1040–1069 ↗(On Diff #62472)

This multiclass definition is unnecessary. Instead, change BrcondPats in MipsInstrInfo.td so that the pattern is using BEQ instead use BEQOp. You can then instantiate the pattern for microMIPS.

1071–1077 ↗(On Diff #62472)

Similarly here. You can extend SetgeImmPats in MipsInstrInfo.td to take and XORiOp parameter.

1082–1086 ↗(On Diff #62472)

With the changes you've made here, some of those patterns (SetlePats, SetgtPats) combine microMIPS and MIPS instructions.

For these multiclass instantiations, extend the definitions in MipsInstroInfo.td to take an XORiOp parameter.

1123–1126 ↗(On Diff #62472)

Align the simm32_relaxed:$imm under GPR32Opnd like:

def : MipsInstAlias<"sltu $rs, $rt, $imm",
                    (SLTiu_MM GPR32Opnd:$rs, GPR32Opnd:$rt,
                              simm32_relaxed:$imm), 0>;
lib/Target/Mips/MipsInstrInfo.td
1694 ↗(On Diff #62472)

Move LUi out of this NotInMicromips block.

test/CodeGen/Mips/brconlt.ll
17 ↗(On Diff #62472)

Formatting: line up the sltius in the immediate block so that they are all in the same column, one space after the longest check prefix. And in the other cases below.

test/CodeGen/Mips/setcc-se.ll
9 ↗(On Diff #62472)

Can you instead match this with "<MCInst #{{[0-9]+}} SLTiu_MM" ? And the other cases below, as it a better match to the expected output.

This revision now requires changes to proceed.Jul 1 2016, 6:49 AM
hvarga updated this revision to Diff 63777.Jul 12 2016, 10:12 PM
hvarga edited edge metadata.

Reworked BrcondPats, SetlePats, SetgePats and SetgeImmPats multiclasses so that they can work for microMips too without interfering with the existing implementations.
Micromips now extends Mips BrcondPats, SeteqPats, SetlePats, SetgtPats, SetgePats and SetgeImmPats multiclasses.
Fixed LUi being in the notInMicroMips predicate block.
Changed the indentations on test files according to the comments.

This update has been contributed by @mamidzic.

sdardis accepted this revision.Jul 21 2016, 2:12 AM
sdardis edited edge metadata.

LGTM with two things: move the BrcondPats out of the NotInMicroMips block, otherwise we get the same bug that this patch addresses, except it affects microMIPS64R6. The second is the placement of a comment. More details inline.

lib/Target/Mips/Mips64InstrInfo.td
539–540 ↗(On Diff #63777)

By moving the pattern instantiation into the block covered by NotInMicroMips, LLVM cannot fall back to picking MIPS64R6 instructions for microMIPS64R6. The instruction mapping tables will correct this for direct object emission and the assembly is identical for microMIPS64R6.

Can you move this out of the NotInMicroMips block?

549 ↗(On Diff #63777)

Place this comment above the 'let AdditionalPredicates..' line.

This revision is now accepted and ready to land.Jul 21 2016, 2:12 AM
This revision was automatically updated to reflect the committed changes.