[PATCH] D122427: [MIPS] Initial support for MIPS-I load delay slots

[Simon Dardis via Phabricator via llvm-commits]

sdardis added a comment.

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.



================
Comment at: llvm/lib/Target/Mips/MipsBranchExpansion.cpp:40
+/// Hazards handled: forbidden slots for MIPSR6, FPU slots for MIPS3 and below,
+/// load slots for MIPS1.
 ///
----------------
Minor nit: 'load slots' -> 'load delay slots'.


================
Comment at: llvm/lib/Target/Mips/MipsBranchExpansion.cpp:807
+  // Load delay slot hazards are only for MIPS1.
+  if (!STI->hasMips1() || STI->hasMips2())
+    return false;
----------------
STI->hasMips2() is enough here.


================
Comment at: llvm/lib/Target/Mips/MipsInstrInfo.cpp:606
+
+  for (const MachineOperand &Op : LoadMI.defs()) {
+    if (Op.isReg() && MIInSlot.readsRegister(Op.getReg()))
----------------
May I suggest:

```
  return !llvm::any_of(LoadMI.defs(), [&](const MachineOperand &Op) {                             
    return Op.isReg() && MIInSlot.readsRegister(Op.getReg());                                     
  });
```


================
Comment at: llvm/lib/Target/Mips/MipsSubtarget.cpp:100
+  }
+  // Don't even attempt to generate code for MIPS-V. It has not
+  // been tested and currently exists for the integrated assembler only.
----------------
Add an empty line before the comment to maintain the style used in the file.


================
Comment at: llvm/test/CodeGen/Mips/mips1-load-delay.ll:1
+; RUN: llc < %s -mtriple=mips -mcpu=mips1 | FileCheck %s -check-prefixes=CHECK,MIPS1
+; RUN: llc < %s -mtriple=mips -mcpu=mips2 | FileCheck %s -check-prefixes=CHECK,MIPS2
----------------
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.


================
Comment at: llvm/test/CodeGen/Mips/mips1-load-delay.ll:7
+; Function Attrs: noinline nounwind optnone
+define dso_local i32 @add_two_pointers(i32* noundef %a, i32* noundef %b) #0 {
+entry:
----------------
You'll need to remove the 'undef' from each the argument attributes, the update_llc_test_checks.py will complain otherwise.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122427/new/

https://reviews.llvm.org/D122427