This is an archive of the discontinued LLVM Phabricator instance.

[MIPS] Initial support for MIPS-I load delay slots
ClosedPublic

Authored by impiaaa on Mar 24 2022, 12:41 PM.

Details

Summary

LLVM so far has only supported the MIPS-II and above architectures. MIPS-II is pretty close to MIPS-I, the major difference
being that "load" instructions always take one extra instruction slot to propogate to registers. This patch adds support for
MIPS-I by adding hazard handling for load delay slots, alongside MIPSR6 forbidden slots and FPU slots, inserting a NOP
instruction between a load and any instruction immediately following that reads the load's destination register. I also
included a simple regression test. Since no existing tests target MIPS-I, those all still pass.

Issue ref: https://github.com/simias/psx-sdk-rs/issues/1

I also tested by building a simple demo app with Clang and running it in an emulator.

Diff Detail

Event Timeline

impiaaa created this revision.Mar 24 2022, 12:41 PM
impiaaa requested review of this revision.Mar 24 2022, 12:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 24 2022, 12:41 PM
impiaaa edited the summary of this revision. (Show Details)Mar 24 2022, 1:09 PM

Hi @impiaaa, thanks for working on this!
Since this is an initial support, I was wondering is there a clear set of TODOs that need to be done?

llvm/lib/Target/Mips/MipsBranchExpansion.cpp
893

nit: . at the end of the comment

899–901

is this clang-formatted?

llvm/lib/Target/Mips/MipsInstrInfo.cpp
600–612

a comment describing the predicate?

I believe it's only marked as initial because of the lack of MIPS-I tests. We assumed we'd have to follow up by porting over the relevant MIPS-II tests. I've also done a fair amount of testing with rustc on emulators and the change works as expected. The only other issue I've found with MIPS-I is that occasionally it'll try emitting a MIPS-II sync that crashes rustc.

Hi @impiaaa, thanks for looking this.

I've highlighted a few minor things that can be improved.

@djtodoro:

I was wondering is there a clear set of TODOs that need to be done?

I believe one issue that needs addressed is that the MipsAsmParser would need to be extended in the case of .set reorder being active, as loads would need to be padded with no-ops. Unfortunately when that directive is active nops need to be added after each load as there is no look-ahead.

llvm/lib/Target/Mips/MipsBranchExpansion.cpp
40

Minor nit: 'load slots' -> 'load delay slots'.

807

STI->hasMips2() is enough here.

llvm/lib/Target/Mips/MipsInstrInfo.cpp
606

May I suggest:

return !llvm::any_of(LoadMI.defs(), [&](const MachineOperand &Op) {                             
  return Op.isReg() && MIInSlot.readsRegister(Op.getReg());                                     
});
llvm/lib/Target/Mips/MipsSubtarget.cpp
100

Add an empty line before the comment to maintain the style used in the file.

llvm/test/CodeGen/Mips/mips1-load-delay.ll
2

You can use the script in llvm/utils/update_llc_test_checks.py to produce the test checks. This is somewhat easier that writing tests by hand.

Remove the 'CHECK' prefixes here, the script will complain if two RUN lines have the same prefixes. As there will only one CHECK prefix, you can change '-check-prefixes' to '-check-prefix'.

You should also add a RUN line with '-mcpu=mips32r2' to add a case where the generate code is not MIPS-I or II.

8

You'll need to remove the 'undef' from each the argument attributes, the update_llc_test_checks.py will complain otherwise.

impiaaa updated this revision to Diff 418435.Mar 26 2022, 9:45 PM

[MIPS] Initial support for MIPS-I load delay slots
Style fixes and comments from review.

impiaaa marked 6 inline comments as done.Mar 26 2022, 9:53 PM

Since this is an initial support, I was wondering is there a clear set of TODOs that need to be done?

I don't actually know right now. I assumed that this is all that was needed for MIPS-I support. I only added the warning in because I hadn't tested on hardware. I'll try to find if there are any other differences between MIPS-I and MIPS-II.

I believe one issue that needs addressed is that the MipsAsmParser would need to be extended in the case of .set reorder being active, as loads would need to be padded with no-ops. Unfortunately when that directive is active nops need to be added after each load as there is no look-ahead.

Should that be a follow-up patch, or included in this one?

llvm/lib/Target/Mips/MipsBranchExpansion.cpp
899–901

Yes.

llvm/test/CodeGen/Mips/mips1-load-delay.ll
2

Is there documentation somewhere on how to get the most out of update_llc_test_checks? I tried running it with this test and it generated assertions for 3 nearly-identical copies of all 28 lines of compiled code, which, in my opinion, is harder to read than assertions that call out the difference.

impiaaa marked an inline comment as not done.Mar 26 2022, 9:54 PM

Should that be a follow-up patch, or included in this one?

It should be a follow up patch.

Is there documentation somewhere on how to get the most out of update_llc_test_checks?

There is some documentation in llvm/doc/TestingGuide.rst, but there's no option to common up the checks. This test case does look small enough that it should be ok.

I'll try to find if there are any other differences between MIPS-I and MIPS-II.

Ok, I did some research. Here are the differences that I can find between MIPS-I and MIPS-II:

  1. Delayed loads: Done.
  2. "Branch likely" instructions: As far as I can tell, these are currently never emitted.
  3. Atomic update: These instructions also do not appear to be emitted.
  4. sync: As @ayrtonm discovered, this instruction can be emitted from atomic loads and stores. It is lowered from the FENCE higher-level instruction. I have a patch ready that expands it to a libcall instead on MIPS-I.
  5. 64-bit coprocessor transfers: Emitted in MipsFastISel::emitLoad, MipsFastISel::emitStore, MipsInstructionSelector::selectLoadStoreOpCode, MipsSEInstrInfo::loadRegFromStack, and MipsSEInstrInfo::storeRegToStack. They can currently be expanded to two single-word transfers with -mno-ldc1-sdc1. This option should be set automatically when MIPS-I is selected, or rather a hasMips2 check should be added everywhere that NoDPLoadStore is also checked.
  6. Trap-on-condition: Emitted in MipsFastISel::selectDivRem, and MipsISelLowering.cpp insertDivByZeroTrap for checking for dividing by zero. The checks could be expanded to separate branch and break instructions. They can also currently be disabled entirely with -mno-check-zero-division.
  7. Native directed rounding of floating-point to fixed-point: trunc_w is emitted in MipsFastISel::selectFPToInt, and MipsInstructionSelector::select. MipsAsmParser::expandTrunc already has code to expand the trunc_w pseudoinstruction, but I don't know if it's used by the assembler.
  8. Floating-point square root: Emitted when -ffast-math is on. I haven't figured out yet where the code makes the decision how to lower the square-root intrinsic—there are other targets that do not have a native square-root instruction that properly expand it to a libcall.

There is some documentation in llvm/doc/TestingGuide.rst, but there's no option to common up the checks. This test case does look small enough that it should be ok.

Now I see that most of the tests in this directory only test the positive case, not both the positive and negative cases like I did. For future tests I'll do that instead.

impiaaa updated this revision to Diff 420019.Apr 2 2022, 4:25 PM

Add mips32r2 to architectures tested by mips1-load-delay.ll

impiaaa marked 3 inline comments as done.Apr 2 2022, 4:26 PM
sdardis accepted this revision.Apr 5 2022, 1:07 PM

LGTM. Do you have commit access?

This revision is now accepted and ready to land.Apr 5 2022, 1:07 PM

Ok, I'll see about committing this tomorrow.

This revision was landed with ongoing or failed builds.Apr 6 2022, 5:04 PM
This revision was automatically updated to reflect the committed changes.

Thank you for the patch.