This is an archive of the discontinued LLVM Phabricator instance.

[mips] MIPSR6 Compact jump support
ClosedPublic

Authored by sdardis on Mar 21 2016, 7:21 AM.

Details

Reviewers
vkalintiris
Summary

This patch adds support for compact jumps similiar to the previous compact
branch support for MIPSR6. Unlike compact branches, compact jumps do not
have a forbidden slot.

As MipsInstrInfo::getEquivalentCompactForm can determine the correct
expansion for jumps and branches for both microMIPS and MIPSR6, simplify
the delay slot filler when producing compact CTIs in cases where a delay slot
can be filled.

Diff Detail

Event Timeline

sdardis updated this revision to Diff 51162.Mar 21 2016, 7:21 AM
sdardis retitled this revision from to [mips] MIPSR6 Compact jump support.
sdardis updated this object.
sdardis added a reviewer: vkalintiris.
sdardis added subscribers: dsanders, llvm-commits.
vkalintiris edited edge metadata.Mar 22 2016, 10:09 AM

Hi Simon, I added some comments inline.

lib/Target/Mips/Mips64r6InstrInfo.td
88

I know that you probably based the definition on the 32-bit description, but do you know why we have to define $at?

122–126

Can you add the relevant MC tests for the new instructions? Either in a new review request or in this one would be fine with me.

lib/Target/Mips/MipsInstrInfo.cpp
260

Can we merge the functionality of this into getEquivalentCompactForm()? As far as I can tell, there's no other user of this member function and there's no point in duplicating the switch statement for every opcode twice.

368–369

I suggest that we move most of these conditions into the initialization of BranchWithZeroOperand in order to simplify the code.

391–392

Why do we need these cases? We don't update the NewOpc when they match.

396–407

What is going to happen to the defined register RA{,_64},

396–429

I believe that we should simplify this by handling the logic in three separate cases: (1) JIALC et al., (2) BranchWithZeroOperand == true, and (3) everything else. I'm afraid that this code could become very complicated in time if we consider all the ABIs & ISAs that we have to support.

413

We can use the convenient MIB.addImm() here.

test/CodeGen/Mips/llvm-ir/ret.ll
21–25

MTHC1 has *unmodeled side-effects* and the DSF bails out on these instructions (see Filler::terminateSearch). Can you update the comment to reflect this?

test/MC/Mips/mips32r6/valid.s
166–167

Indentation.

dsanders added inline comments.Mar 29 2016, 3:58 AM
lib/Target/Mips/Mips64r6InstrInfo.td
88

I haven't dug into the details of exactly what can/can't cause it and I don't know whether it applies to this instruction or not but the explanation I was given a couple years ago was that $at may be clobbered as a result of changes made by the linker (e.g. out-of-range jumps, plt stubs, etc.).

test/CodeGen/Mips/llvm-ir/ret.ll
21–25

MTHC1 shouldn't have side-effects, it looks like tablegen is inferring it from the lack of a codegen pattern.

sdardis updated this revision to Diff 51898.Mar 29 2016, 6:58 AM
sdardis edited edge metadata.
sdardis marked 10 inline comments as done.

Response for reviewer comments.

sdardis added inline comments.Mar 29 2016, 7:07 AM
lib/Target/Mips/Mips64r6InstrInfo.td
88

After some discussion with Daniel, it's necessary as the long branch pass and some possible linker stubs clobber $at.

122–126

Are they not covered by test/MC/Mips/mips64r6/valid.s, line 145 & 146?

lib/Target/Mips/MipsInstrInfo.cpp
396–407

RA(_64) will be marked as implicit def/dead as required since its flags are copied from the original instruction. Not erasing that operand will result in contradictory information about RA(_64), as adding JIALC/JIALIC64 will mark %RA<imp-def>, however the function call will mark %RA<imp-def,dead>.

Looking at the MIR statement:
JIALC64 %T9_64, 0, <regmask %FP %GP %RA %D12 %D13 %D14 %D15 %F24 %F25 %F26 %F27 %F28 %F29 %F30 %F31 %FP_64 %F_HI24 %F_HI25 %F_HI26 %F_HI27 %F_HI28 %F_HI29 %F_HI30 %F_HI31 %GP_64 %RA_64 %S0 %S1 %S2 %S3 %S4 %S5 %S6 %S7 %D24_64 %D25_64 %D26_64 %D27_64 %D28_64 %D29_64 %D30_64 %D31_64 %S0_64 %S1_64 %S2_64 %S3_64 %S4_64 %S5_64 %S6_64 %S7_64>, %RA<imp-def>, %RA<imp-def,dead>, %GP_64<imp-use>, %SP<imp-def>, %V0<imp-def>

%RA appears twice, with the flags duplicated partially. Hence why we have to delete it.

vkalintiris accepted this revision.Mar 31 2016, 8:53 AM
vkalintiris edited edge metadata.

LGTM with some minor inline comments. Clang-format before committing this, as I've spotted 2-3 cases with wrong indentation.

lib/Target/Mips/Mips64r6InstrInfo.td
122–126

Sorry, I missed the codegen-only bit.

lib/Target/Mips/MipsDelaySlotFiller.cpp
582

s/Pseduo/Pseudo/

lib/Target/Mips/MipsInstrInfo.cpp
263–270

There are way several conditions here. Can you replace this with a switch statement similar to the one you wrote in CTIHasCompactMMEncoding()?

This revision is now accepted and ready to land.Mar 31 2016, 8:53 AM
sdardis closed this revision.Apr 5 2016, 11:54 AM

Committed as r265390.