This is an archive of the discontinued LLVM Phabricator instance.

[mips] Enable tail calls by default
ClosedPublic

Authored by sdardis on Jun 8 2016, 8:07 AM.

Details

Summary

Enable tail calls by default for (micro)MIPS(64).

microMIPS is slightly more tricky than doing it for MIPS(R6) or microMIPSR6.
microMIPS has two instruction encodings: 16bit and 32bit along with some
restrictions on the size of the instruction that can fill the delay slot.
For safe tail calls for microMIPS, the delay slot filler attempts to find
a correct size instruction for the delay slot of TAILCALL pseudos.

Diff Detail

Repository
rL LLVM

Event Timeline

sdardis updated this revision to Diff 60040.Jun 8 2016, 8:07 AM
sdardis retitled this revision from to [mips] Enable tail calls by default.
sdardis updated this object.
sdardis added a reviewer: dsanders.
sdardis set the repository for this revision to rL LLVM.
sdardis added a subscriber: llvm-commits.
sdardis updated this object.Jun 8 2016, 8:07 AM
sdardis edited edge metadata.
sdardis updated this revision to Diff 61531.Jun 22 2016, 4:57 AM
sdardis removed rL LLVM as the repository for this revision.

Changed TailCallReg (all variants) to use jr $dst over jalr zero, $dst. This should avoid mispredicts on implementations with a return address predictor.

dsanders requested changes to this revision.Jun 30 2016, 7:43 AM
dsanders edited edge metadata.

Most of my comments are nits. The important one is the accidental removal of predicates on the new instructions with a 'let Predicates = ...' statement.

lib/Target/Mips/MicroMipsInstrInfo.td
490 ↗(On Diff #61531)

indentation

993–995 ↗(On Diff #61531)

This doesn't have the predicates you expect because the 'let Predicates = ...' statement discards all the predicates and replaces them with the 1-element list '[InMicroMips]'. The effects of ISA_MIPS1_NOT_32R6_64R6 are lost.

Changing Predicates to AdditionalPredicates should fix this for now but we need to move InMicroMips to another sublist of PredicateControl in the future.

The other instances should be fixed too but that's for another patch

lib/Target/Mips/MipsDelaySlotFiller.cpp
561 ↗(On Diff #60040)

Indentation

617–622 ↗(On Diff #60040)

becuase -> because

Also, the sentence starting 'This would save 16 bits ...' seems to be missing some words.

709–713 ↗(On Diff #60040)

I see that 'j' is the tailcall instruction at the moment. I agree that it's fairly safe to rely on this instruction since it has a 256MB range but it's important to mention that this range isn't centered on the instruction like a normal PC-relative instruction. It's technically possible for small offsets like +/-4 to be out of range. JAL has the same issue so I think this is a problem that can be addressed later.

Also, a couple nits:

  • Could you clarify that the '32bit' is the opcode size and not the range?
  • ranage -> range.
lib/Target/Mips/MipsSEISelLowering.cpp
30–31 ↗(On Diff #61531)

I think we should drop the 'enable' and 'disable' (maybe have 'Use' instead) and just have:

static cl::opt<bool>
MipsTailCalls("mips-tail-calls", cl::Hidden,
              cl::desc("MIPS: Disable tail calls."), cl::init(true));

That way we control the default with cl::init and override it with -mips-tail-calls=false

test/CodeGen/Mips/compactbranches/tailrecursion.ll
23 ↗(On Diff #61531)

This matches the first 'f' in the file (e.g. .file) rather than the label 'f' at the start of the function. It needs a ':'

33 ↗(On Diff #61531)

menomic -> mnemonic

test/CodeGen/Mips/i64arg.ll
5–25 ↗(On Diff #61531)

We shouldn't need to switch the R2/R3 variables over since CHECK-DAG takes care of ordering differences.

This revision now requires changes to proceed.Jun 30 2016, 7:43 AM
sdardis updated this revision to Diff 62662.Jul 4 2016, 5:14 AM
sdardis edited edge metadata.
sdardis marked 6 inline comments as done.

Update as per review comments.

I will see about changing the usage of branch instead of jump instructions in a later patch.

sdardis updated this revision to Diff 62747.Jul 5 2016, 7:21 AM
sdardis edited edge metadata.

Missed the necessary -target-abi n64 option on some of the tests.

sdardis updated this revision to Diff 64487.Jul 19 2016, 7:09 AM

Rebase, ping.

dsanders accepted this revision.Aug 2 2016, 1:53 AM
dsanders edited edge metadata.

LGTM

This revision is now accepted and ready to land.Aug 2 2016, 1:53 AM
This revision was automatically updated to reflect the committed changes.