[PATCH] D39115: [MIPS][MicroMIPS] Extending size reduction pass with LWP and SWP

[Simon Dardis via Phabricator via llvm-commits]

sdardis requested changes to this revision.
sdardis added a comment.
This revision now requires changes to proceed.

Can you also provide some tests than ensure that lwp/swp is not formed when the first register is ra? You'll need to use some mir based testing (i.e. compile an ll file and use -stop-before= and the list test will use -start-after=)



================
Comment at: lib/Target/Mips/MicroMipsSizeReduction.cpp:86
+  enum ReduceType eRType;                ///< Reduction type
+  bool (*ReduceFunction)(void *FunArgs); ///< Pointer to reduce function
+  struct OpCodes Ops;                    ///< All relevant OpCodes
----------------
You should not be using void * pointers. Instead, provide a typedef for the structure you're passing and use that instead.


================
Comment at: lib/Target/Mips/MicroMipsSizeReduction.cpp:116-128
+// Function arguments for ReduceFunction
+struct ReduceEntryFunArgs {
+  MachineInstr *MI;         // Instruction
+  const ReduceEntry &Entry; // Entry field
+  MachineBasicBlock::instr_iterator
+      &NextMII; // Iterator to next instruction in block
+  const MachineBasicBlock::instr_iterator &E; // End iterator
----------------
Provide a forward declaration and typedef for this structure above 'struct ReduceEntry', as void * should not be used.


================
Comment at: lib/Target/Mips/MicroMipsSizeReduction.cpp:189
+                                 MachineInstr *MI2 = nullptr,
+                                 bool flag = false);
 
----------------
The parameter flag should have a more descriptive name.


================
Comment at: lib/Target/Mips/MicroMipsSizeReduction.cpp:441-445
+  MachineInstr *MI1 = Arguments.MI;
+  MachineInstr *MI2 = nullptr;
+
+  if (NextMII == E)
+    return false;
----------------
Swap these two blocks in order and you can the set MI2 to NextMII immediately.


================
Comment at: lib/Target/Mips/MicroMipsSizeReduction.cpp:639-644
+        // FIXME: This should be MIB.add(MI2->getOperand(0));
+        // However, TabGen counts regpair as one output operand,
+        // and that introduces machine verfier error for lwp instruction.
+        // Setting the second register as undef, bypasses this error.
+        // This should not introduce bugs as this pass is run
+        // immediately before machine code is emitted.
----------------
Unfortunately this is not the case that this pass is among the last to run before machine code is emitted. This pass runs before the delay slot filler+long branch+hazard scheduling. There are other general later passes which do some analysis for stackmaps and debug information and funclet layout and the like.

A particular problem here is that as the lwp/swp instructions either write/read their operands the delay slot filler will not be able to or will incorrect analyse that for the purposes of filling delay slots.

The second problem is that lwp and swp are unpredictable when placed in delay slots and the delay slot filler doesn't know that.

There are number of issues to be resolved here: correcting the definition of lwp and swp so that their definition reflects their actual operation; marking this instructions in some way as being unsuitable for being placed in delay slots.


================
Comment at: lib/Target/Mips/MicroMipsSizeReduction.cpp:684
   // TODO: Add support for other subtargets:
-  // microMIPS32r6 and microMIPS64r6
+  // microMIPS32r6
   if (!Subtarget->inMicroMipsMode() || !Subtarget->hasMips32r2() ||
----------------
Unnecessary change.


https://reviews.llvm.org/D39115