This is an archive of the discontinued LLVM Phabricator instance.

[mips] Support SELECT nodes for targets that don't have conditional-move instructions.
ClosedPublic

Authored by vkalintiris on Nov 11 2014, 5:32 AM.

Details

Summary

For Mips targets that do not have conditional-move instructions, ie. targets
before MIPS32 and MIPS-IV, we have to insert a diamond control-flow
pattern in order to support SELECT nodes. In order to do that, we add
pseudo-instructions with a custom inserter that emits the necessary
control-flow that selects the correct value.

With this patch we add complete support for code generation of Mips-II targets
based on the LLVM test-suite.

Diff Detail

Repository
rL LLVM

Event Timeline

vkalintiris retitled this revision from to [mips] Support SELECT nodes for targets that don't have conditional-move instructions..
vkalintiris updated this object.
vkalintiris edited the test plan for this revision. (Show Details)
vkalintiris added a reviewer: dsanders.
vkalintiris added a subscriber: Unknown Object (MLST).
dsanders requested changes to this revision.Nov 12 2014, 3:09 AM
dsanders edited edge metadata.

Great! With this, we will be able to generate code for the mips/mipsel ports of Debian.

There are quite a few comments below but most are minor things. The main one is about whether we really need the SelectFP_[TF] nodes.

lib/Target/Mips/MipsCondMov.td
269 ↗(On Diff #16043)

Don't override the Predicates field, it has some surprising results since it's very easy to accidentally remove predicates. Instead override InsnPredicates, preferably using an ISA_* or INSN_* adjective. See the PredicateControl class for more information.

lib/Target/Mips/MipsISelLowering.cpp
536–537 ↗(On Diff #16043)

Given that CMovFP_[TF] and SelectFP_[TF] have the same inputs and outputs, do we really need new nodes? Couldn't we use CMovFP_[TF] and use predicates to pick between the CMov instructions (added in Mips4 and Mips32) and the new pseudo's?

lib/Target/Mips/MipsInstrInfo.td
189 ↗(On Diff #16043)

Nit: Based on the names of the other predicates this should either be NoCondMov or NotCondMov but, as noted above, it would be better to define and use an INSN_* or ISA_* adjective (e.g. ISA_NOT_4_32).

lib/Target/Mips/MipsSubtarget.h
199–202 ↗(On Diff #16043)

This won't be needed once you do the above comments.

test/CodeGen/Mips/llvm-ir/select.ll
39 ↗(On Diff #16043)

movn from the CMOV case has tied operands but this is not the case for seleqz/selnez. As a result, the first [[T0]] on this line isn't guaranteed to be the same register as the second [[T0]]. You should define another variable.

Likewise for the other tests below.

100–111 ↗(On Diff #16043)

Please add a FIXME commenting on the redundant control flow.

114 ↗(On Diff #16043)

Please check the offsets as well.

Likewise in the tests below.

134 ↗(On Diff #16043)

This instruction is redundant. Could you add a FIXME comment?

142–173 ↗(On Diff #16043)

I should mention that for MIPS-II, this is testing selection between two GPR's. This is because of a quirk of the O32 ABI that prevents the use of FPR's if there is a non-floating point argument as the first argument. MIPS32 is testing selection between FPR's since it's copying $5 and $6 to $f0, and $f1 first.

We should duplicate this test with the arguments re-ordered (%x, %y, %s) so that we test the FPR case for 32-bit ISA's.

Likewise for the double test below.

327 ↗(On Diff #16043)

Nit: Too many blank lines

395 ↗(On Diff #16043)

Nit: Too many blank lines

516 ↗(On Diff #16043)

Nit: Too many blank lines

This revision now requires changes to proceed.Nov 12 2014, 3:09 AM
vkalintiris edited edge metadata.

Use CMovFP nodes for Select_Pseudo instructions and update test to address Daniel's comments.

vkalintiris edited edge metadata.

I replaced BC1T with BC1F and vice versa, because we were branching on the
wrong condition.

dsanders accepted this revision.Dec 11 2014, 3:23 AM
dsanders edited edge metadata.

LGTM. Thanks.

lib/Target/Mips/MipsCondMov.td
269 ↗(On Diff #16043)

Done

lib/Target/Mips/MipsISelLowering.cpp
536–537 ↗(On Diff #16043)

Done

lib/Target/Mips/MipsInstrInfo.td
189 ↗(On Diff #16043)

Done

test/CodeGen/Mips/llvm-ir/select.ll
39 ↗(On Diff #16043)

Done

100–111 ↗(On Diff #16043)

Done

114 ↗(On Diff #16043)

Done

134 ↗(On Diff #16043)

Done

142–173 ↗(On Diff #16043)

Done

327 ↗(On Diff #16043)

Done

395 ↗(On Diff #16043)

Done

516 ↗(On Diff #16043)

Done

This revision is now accepted and ready to land.Dec 11 2014, 3:23 AM
This revision was automatically updated to reflect the committed changes.