This is an archive of the discontinued LLVM Phabricator instance.

Removal of microMIPS64R6
ClosedPublic

Authored by abeserminji on Jul 19 2017, 8:42 AM.

Details

Summary

All files and parts of files related to microMips64r6 are removed.
When target is micromips64r6, errors are printed.

This is LLVM part of patch. Clang part of patch can be found here: https://reviews.llvm.org/D35624

Depends on: D37682 and D37683

Diff Detail

Event Timeline

abeserminji created this revision.Jul 19 2017, 8:42 AM
abeserminji edited the summary of this revision. (Show Details)Jul 19 2017, 8:43 AM
sdardis edited edge metadata.Jul 25 2017, 6:43 AM
sdardis added a subscriber: llvm-commits.

Some minor comments.

CodeGen/Mips/tailcall/tailcall.ll
31 ↗(On Diff #107303)

Spurious blank line.

Target/Mips/MipsSubtarget.cpp
107 ↗(On Diff #107303)

The error message should be "microMIPS64R6 is not supported".

Target/Mips/MipsTargetMachine.cpp
181 ↗(On Diff #107303)

mircomips -> microMIPS.

abeserminji marked 3 inline comments as done.Jul 25 2017, 7:26 AM

Comments resolved.

Comments resolved.

There's also a mention of micromips64r6 in lib/Target/Mips/Relocations.txt, so that file needs to be kept up to date.

Target/Mips/MicroMips64r6InstrInfo.td
481–482 ↗(On Diff #108077)

This multiclass can be flattened as it is no longer required.

sdardis requested changes to this revision.Sep 6 2017, 2:05 AM

Flagging this as changes required.

This revision now requires changes to proceed.Sep 6 2017, 2:05 AM
abeserminji edited edge metadata.
abeserminji marked an inline comment as done.
abeserminji edited the summary of this revision. (Show Details)

Relocation.txt updated.
Multiclass MipsHighestHigherHiLoRelocs flattened.

Added dependencies.

Last revision also included some changes on test/MC/Mips/mips64r6/invalid.s, which should not be in this patch.

Patch updated to follow latest revision.

sdardis requested changes to this revision.Sep 20 2017, 5:59 AM

The GPRMM16_64 register class needs to be removed as well. It's used in the MipsMachineFunction.cpp and MipsRegisterInfo.cpp.

lib/Target/Mips/MipsDelaySlotFiller.cpp
601–602 ↗(On Diff #115401)

"This is the microMIPS32R6 subtarget which does not have instructions which have delay slots."

This revision now requires changes to proceed.Sep 20 2017, 5:59 AM
sdardis added inline comments.Sep 20 2017, 7:46 AM
lib/Target/Mips/Disassembler/MipsDisassembler.cpp
1070–1071

Unused variable.

1110–1111

Unused variable.

abeserminji edited edge metadata.
abeserminji marked 3 inline comments as done.

Comments resolved.

sdardis requested changes to this revision.Sep 26 2017, 3:51 AM

Can you also remove the instruction mappings for the R6 variants of dbitswap, dclo, dclz, dlsa, lld, lwupc and ldpc ?

lib/Target/Mips/Mips64InstrInfo.td
102

This instruction no longer has a corresponding microMIPSR6 mapping, so StdMMR6Rel can be removed here.

123–129

These instructions no longer have a corresponding microMIPSR6 mapping, so StdMMR6Rel can be removed here.

144–178

These instructions no longer have a corresponding microMIPSR6 mapping, so StdMMR6Rel can be removed here.

194–201

These instructions no longer have a corresponding microMIPSR6 mapping, so StdMMR6Rel can be removed here.

224

This instruction no longer has a corresponding microMIPSR6 mapping, so StdMMR6Rel can be removed here.

302–305

These instructions no longer have a corresponding microMIPSR6 mapping, so StdMMR6Rel can be removed here.

This revision now requires changes to proceed.Sep 26 2017, 3:51 AM
abeserminji edited edge metadata.
abeserminji marked 6 inline comments as done.

Removed unnecessary annotations.

btw. out of curiosity haven't these StdMMR6Rel's been in contradiction with NotInMicroMips additional predicate?

For SelectionDAGISel purposes, these instruction mappings were spurious as SelectionDAGISel would pick the corresponding microMIPS instruction due to the various predicates. If however, these instructions were introduced by the likes of MipsSEISelLowering or MipsSEISelDAGToDAG, etc, we'd end up with a mixture of MIPS and microMIPS code. This isn't a problem when producing assembly, as the textual representation is the same, but for object emission we need to produce the correct opcodes. The instruction mapping tables quietly "fixup" the usage of MIPS opcodes in microMIPS code which simplifies injecting instructions by hand in cpp as we don't have to constantly do things like:

BuildMI(MBB, I, DL, TII.get(STI.inMicroMips ? Mips::DADDU_MMR6 : Mips::DADDu), V1).addReg(V0)

throughout the codebase.

Can you remove the StdMMR6Rel from the R6 instructions dbitswap, dclo, dclz, dlsa, lld, lwupc and ldpc ?

Comment resolved.

sdardis requested changes to this revision.Oct 13 2017, 6:05 AM

Probably my last round of comments on this, also can you add a trivial test case showing that microMIPS64R6 is not supported?

lib/Target/Mips/MicroMipsInstrInfo.td
590–602

Align the format class instantiations / ISA predicates under instruction codegen description instantiations.

650–652

The ISA_MICROMIPS_NOT_32R6 needs to be aligned under the BrkSdbbp16MM.

lib/Target/Mips/MicroMipsSizeReduction.cpp
498

for the subtarget microMIPS32R6.

lib/Target/Mips/MipsTargetMachine.cpp
188–192 ↗(On Diff #118596)

Take the context from the current Function and use emitError giving the function name + error message.

See include/llvm/IR/LLVMContext.h:300~ for emitError.

test/CodeGen/Mips/llvm-ir/and.ll
42–43

Remove this.

57–58

Remove this.

72–73

Remove this.

test/CodeGen/Mips/llvm-ir/mul.ll
248–249

Remove this.

test/CodeGen/Mips/llvm-ir/not.ll
131–132

Remove this.

144–145

Remove this.

157–158

Remove this.

test/CodeGen/Mips/llvm-ir/or.ll
32

Remove this.

44–45

Remove this.

59–60

Remove this.

test/CodeGen/Mips/llvm-ir/xor.ll
43

Remove this.

55–56

Remove this.

70–71

Remove this.

This revision now requires changes to proceed.Oct 13 2017, 6:05 AM
abeserminji edited edge metadata.
abeserminji marked 14 inline comments as done.

Added test that tests that microMIPS64R6 is not supported anymore.
Comments resolved.

abeserminji added inline comments.Oct 17 2017, 4:12 AM
lib/Target/Mips/MipsTargetMachine.cpp
188–192 ↗(On Diff #118596)

I removed this check here, because I was not able to reproduce the error at this place. The check in MipsSubtarget.cpp at 107 always performs first, so this seemed to me like unnecessary thing. Although, correct me if I'm wrong, I might missed some case where this is called before the mentioned check in MipsSubtarget.cpp.

I missed the AsmParser in my last reviews, can you add checks there, in the initialisation, in parseSetArchDirective and parseDirectiveSet?

lib/Target/Mips/MipsSubtarget.cpp
108

Pass false as the second argument here, so that we don't emit a crash diagnostic.

lib/Target/Mips/MipsTargetMachine.cpp
188–192 ↗(On Diff #118596)

As far as I can see, this check can be dropped.

test/CodeGen/Mips/micromips-64r6-disabled.ll
1 ↗(On Diff #119281)

This file should be called micromips64r6-unsupported.ll.

3 ↗(On Diff #119281)

Test that microMIPS64R6 is not supported.

abeserminji marked 6 inline comments as done.

Comments addressed.

You've missed my comment:

I missed the AsmParser in my last reviews, can you add checks there, in the initialisation, in parseSetArchDirective and parseDirectiveSet?

Hi Simon,
Sorry about missing your comment.
I added necessary checks, but I added only for two cases:

  1. when cpu=mips64r6 and micromips option is used
  2. when cpu=mips64r6 and micromips directive is used

I couldn't find any other cases, so I added checks to the constructor and parseDirectiveSet(). Is there some case that I missed that could call parseSetArchDirective() without triggering current checks?
Also, I added a test that checks these errors.

.set  micromips
.set  arch=mips64r6
nop

covers the parseSetArchDirective case.

lib/Target/Mips/AsmParser/MipsAsmParser.cpp
516

Don't use STI directly here, use getSTI() instead.

7163

Check here if the assembler is in micromips mode.

abeserminji marked 2 inline comments as done.

Comments addressed.

abeserminji added inline comments.Nov 22 2017, 7:25 AM
test/MC/Mips/micromips64r6-unsupported.s
15 ↗(On Diff #123941)

I made different error messages here on purpose, so that it is clear which part of code is tested.

sdardis accepted this revision.Dec 6 2017, 7:34 AM

LTGM with the inlined nits addressed, along with any other changes that have to be made during rebasing before commit provided they're obvious/minor.

lib/Target/Mips/AsmParser/MipsAsmParser.cpp
7124

".set micromips directive is not supported with MIPS64R6"

7160

"MIPS64R6 is not supported with microMIPS"

This revision is now accepted and ready to land.Dec 6 2017, 7:34 AM

Comments resolved.
Rebase against latest master.

Last update doesn't contain deleted files. Fixed that.

This revision was automatically updated to reflect the committed changes.