[PATCH] D15144: [mips[microMIPS]] Adding code size reduction pass for MicroMIPS
Daniel Sanders via llvm-commits
llvm-commits at lists.llvm.org
Wed Mar 23 06:13:49 PDT 2016
dsanders added a comment.
Sorry, I was part way through writing them a few weeks ago but was distracted by other things.
There appears to be two optimizations in this pass with very different requirements at the moment. The first optimization is a simple substitution of an MI for an equivalent MI with a smaller encoding. This part is generally heading in the right direction. The second is a peephole optimization that reduces two or more MI's into a single MI and this is where most of my concerns are. I don't believe it's checking enough to be able to prove that this reduction is safe. For example, ReduceMIToLwpSwp checks for interfering register uses but fails to check for interfering register defs (including implicit defs and sub/super-registers), memory reads/writes (including aliases), volatile accesses, side effects, etc. I think we should remove this portion for now and proceed with the simple size reductions to begin with.
For the testing in general: We ought to make use of the MIR (http://llvm.org/docs/MIRLangRef.html) so that we're only testing this pass. However, I'm not going to make that a requirement for this patch because I haven't used it myself yet.
I haven't looked too closely at the test cases yet but they will need to check the operands since this is a key part of whether your optimization is working as intended. I'd also like the tests to be more focused than they currently are. They look like they were generated from C examples and as such have a lot of unnecessary noise.
The rest of the comments below are things I noted while reading the patch. I've included them because I've already written them but some will most likely be made moot by the above changes. If the line numbers seem odd it's because they were written for the previous diff.
================
Comment at: lib/Target/Mips/MicroMips32SizeReduction.cpp:40-43
@@ +39,6 @@
+// Reduction type:
+// SeveralInstr - several instructions into lwm/swm
+// TwoInstr - two instructions into one
+// OneInstr - 32-bit instruction into 16-bit instruction
+enum ReduceType { SeveralInstr, TwoInstr, OneInstr };
+
----------------
We can make this explanation appear in the doxygen documentation by writing this with '///' and '///<' comments like so:
/// Reduction type:
enum ReduceType {
SeveralInstr, ///< Several instructions into lwm/swm.
TwoInstr, ///< Two instructions into one.
OneInstr ///< 32-bit instruction into 16-bit instruction.
};
Similarly for the other description comments below.
I notice that our doxygen config currently has 'EXTRACT_ANON_NSPACES=NO' but I'm going to propose that we change that.
================
Comment at: lib/Target/Mips/MicroMips32SizeReduction.cpp:47
@@ +46,3 @@
+struct ImmField {
+ ImmField() : ImmFieldOpperand(-1), Shift(0), LBound(0), HBound(0) {}
+ ImmField(uint8_t sh, int16_t lb, int16_t hb, int8_t immf)
----------------
Opperand -> Operand. This typo appears in a few other places too
================
Comment at: lib/Target/Mips/MicroMips32SizeReduction.cpp:48
@@ +47,3 @@
+ ImmField() : ImmFieldOpperand(-1), Shift(0), LBound(0), HBound(0) {}
+ ImmField(uint8_t sh, int16_t lb, int16_t hb, int8_t immf)
+ : ImmFieldOpperand(immf), Shift(sh), LBound(lb), HBound(hb) {}
----------------
Variables should begin with a capital and should be descriptive. With this style of constructor it's not ambiguous to use the same name for both the argument and the member (e.g. 'Shift(Shift)').
Similarly for the other constructors below.
================
Comment at: lib/Target/Mips/MicroMips32SizeReduction.cpp:59
@@ +58,3 @@
+struct OpInfo {
+ OpInfo(enum opNum to, bool snr = false)
+ : TransferOperands(to), SmallerNumRegs(snr) {}
----------------
The snr argument is never used.
================
Comment at: lib/Target/Mips/MicroMips32SizeReduction.cpp:64
@@ +63,3 @@
+ enum opNum TransferOperands; // Operands to transfer to the new instruction
+ bool SmallerNumRegs; // In 16 bit instr a smaller num of registers is used
+};
----------------
I think I know what you're trying to say but the comment isn't very clear. I think you're referring to the way LWM16 only allows a subset of the registers that LVM32 allows.
Can we describe this in terms of register classes?
================
Comment at: lib/Target/Mips/MicroMips32SizeReduction.cpp:73-75
@@ +72,5 @@
+
+ uint16_t WideOpc; // Wide opcode
+ uint16_t Opc2; // Opcode of a second instruction
+ uint16_t NarrowOpc; // Narrow opcode
+};
----------------
We normally use 'unsigned' for opcodes.
Also, what's the purpose of the second instruction? It's not clear from the comment
================
Comment at: lib/Target/Mips/MicroMips32SizeReduction.cpp:83
@@ +82,3 @@
+ enum ReduceType eRType; // Several instr. to one, Two instr. to one, 32 to 16
+ bool (*ReduceFunction)(void *v); // Pointer to reduce function
+ struct OpCodes Ops; // All relevant OpCodes
----------------
Why 'void *'? It seems we always pass in a 'struct ReduceEntryFA *'. We also de-reference it and take a copy immediately without nullptr checks so a reference would be better to avoid the copy and explicitly say it can't be nullptr at the same time.
================
Comment at: lib/Target/Mips/MicroMips32SizeReduction.cpp:124-125
@@ +123,4 @@
+ const MachineBasicBlock::instr_iterator &E; // End iterator
+ MachineBasicBlock::instr_iterator
+ &NNextMII; // Iterator to next instruction, if
+ const ReduceEntry &Entry; // Entry field
----------------
Formatting.
================
Comment at: lib/Target/Mips/MicroMips32SizeReduction.cpp:143
@@ +142,3 @@
+
+class MicroMips32SizeReduce : public MachineFunctionPass {
+public:
----------------
Naming nit: We should probably drop the '32' so that we can re-use it for microMIPS64 in the future.
================
Comment at: lib/Target/Mips/MicroMips32SizeReduction.cpp:158
@@ +157,3 @@
+private:
+ /// ReduceMBB - Reduces width of instructions in the specified basic block.
+ bool ReduceMBB(MachineBasicBlock &MBB);
----------------
New code shouldn't repeat the function name in the comments.
================
Comment at: lib/Target/Mips/MicroMips32SizeReduction.cpp:207-208
@@ +206,4 @@
+
+ // Table with transformation rules for each instruction
+ static std::vector<ReduceEntry> ReduceTable;
+};
----------------
Given that this is a static table, we should define the table as a normal array and use ArrayRef in this class
================
Comment at: lib/Target/Mips/MicroMips32SizeReduction.cpp:212
@@ +211,3 @@
+char MicroMips32SizeReduce::ID = 0;
+const MipsInstrInfo *MicroMips32SizeReduce::MipsII;
+
----------------
Is this redundant?
================
Comment at: lib/Target/Mips/MicroMips32SizeReduction.cpp:214-269
@@ +213,58 @@
+
+std::vector<ReduceEntry> MicroMips32SizeReduce::ReduceTable = {
+
+ // ReduceType, OpCodes, ReduceFunction,
+ // OpInfo(TransferOperands, SmallerNumRegs=false),
+ // ImmField(Shift, LBound, HBound, ImmFieldPosition)
+
+ {OneInstr, OpCodes(Mips::LWM32_MM, Mips::LWM16_MM), ReduceLoadStore,
+ OpInfo(opAll), ImmField(2, 0, 16, 2)},
+ {OneInstr, OpCodes(Mips::LWM_MM, Mips::LWM16_MM), ReduceLoadStore,
+ OpInfo(opAll), ImmField(2, 0, 16, 2)},
+ {OneInstr, OpCodes(Mips::LW_MM, Mips::LWSP_MM), ReduceLWtoLWSP,
+ OpInfo(opAll), ImmField(2, 0, 32, 2)},
+ {OneInstr, OpCodes(Mips::LW, Mips::LWSP_MM), ReduceLWtoLWSP, OpInfo(opAll),
+ ImmField(2, 0, 32, 2)},
+ {OneInstr, OpCodes(Mips::SW_MM, Mips::SW16_MM), ReduceSWtoSW16,
+ OpInfo(opAll), ImmField(2, 0, 16, 2)},
+ {OneInstr, OpCodes(Mips::SW, Mips::SW16_MM), ReduceSWtoSW16, OpInfo(opAll),
+ ImmField(2, 0, 16, 2)},
+ {OneInstr, OpCodes(Mips::SW_MM, Mips::SWSP_MM), ReduceSWtoSWSP,
+ OpInfo(opAll), ImmField(2, 0, 32, 2)},
+ {OneInstr, OpCodes(Mips::SW, Mips::SWSP_MM), ReduceSWtoSWSP, OpInfo(opAll),
+ ImmField(2, 0, 32, 2)},
+ {OneInstr, OpCodes(Mips::SWM32_MM, Mips::SWM16_MM), ReduceLoadStore,
+ OpInfo(opAll), ImmField(2, 0, 16, 2)},
+ {OneInstr, OpCodes(Mips::SWM_MM, Mips::SWM16_MM), ReduceLoadStore,
+ OpInfo(opAll), ImmField(2, 0, 16, 2)},
+
+ // Transfer two instructions into one
+ {TwoInstr, OpCodes(Mips::LW, Mips::LW, Mips::LWP_MM), ReduceMIToLwpSwp,
+ OpInfo(opLwpSwp), ImmField(0, -2048, 2048, 2)},
+ {TwoInstr, OpCodes(Mips::LW, Mips::LW_MM, Mips::LWP_MM), ReduceMIToLwpSwp,
+ OpInfo(opLwpSwp), ImmField(0, -2048, 2048, 2)},
+ {TwoInstr, OpCodes(Mips::LW_MM, Mips::LW, Mips::LWP_MM), ReduceMIToLwpSwp,
+ OpInfo(opLwpSwp), ImmField(0, -2048, 2048, 2)},
+ {TwoInstr, OpCodes(Mips::LW_MM, Mips::LW_MM, Mips::LWP_MM),
+ ReduceMIToLwpSwp, OpInfo(opLwpSwp), ImmField(0, -2048, 2048, 2)},
+
+ {TwoInstr, OpCodes(Mips::SW, Mips::SW, Mips::SWP_MM), ReduceMIToLwpSwp,
+ OpInfo(opLwpSwp), ImmField(0, -2048, 2048, 2)},
+ {TwoInstr, OpCodes(Mips::SW, Mips::SW_MM, Mips::SWP_MM), ReduceMIToLwpSwp,
+ OpInfo(opLwpSwp), ImmField(0, -2048, 2048, 2)},
+ {TwoInstr, OpCodes(Mips::SW_MM, Mips::SW_MM, Mips::SWP_MM),
+ ReduceMIToLwpSwp, OpInfo(opLwpSwp), ImmField(0, -2048, 2048, 2)},
+ {TwoInstr, OpCodes(Mips::SW_MM, Mips::SW, Mips::SWP_MM), ReduceMIToLwpSwp,
+ OpInfo(opLwpSwp), ImmField(0, -2048, 2048, 2)},
+
+ // Transfer several instructions into one
+ {SeveralInstr, OpCodes(Mips::LW, Mips::LWM_MM), ReduceMIToLWMSWM,
+ OpInfo(NA), ImmField(0, -2048, 2048, 2)},
+ {SeveralInstr, OpCodes(Mips::LW_MM, Mips::LWM_MM), ReduceMIToLWMSWM,
+ OpInfo(NA), ImmField(0, -2048, 2048, 2)},
+ {SeveralInstr, OpCodes(Mips::SW, Mips::SWM_MM), ReduceMIToLWMSWM,
+ OpInfo(NA), ImmField(0, -2048, 2048, 2)},
+ {SeveralInstr, OpCodes(Mips::SW_MM, Mips::SWM_MM), ReduceMIToLWMSWM,
+ OpInfo(NA), ImmField(0, -2048, 2048, 2)},
+};
+}
----------------
This table should probably be tablegen-erated but we can leave that for now and address it in later patches.
================
Comment at: lib/Target/Mips/MicroMips32SizeReduction.cpp:272-288
@@ +271,19 @@
+
+// Returns true if the register Reg is $16, $17, or $2-$7.
+static bool isMMThreeBitGPRegister(unsigned Reg) {
+ using namespace Mips;
+ switch (Reg) {
+ case S0:
+ case S1:
+ case V0:
+ case V1:
+ case A0:
+ case A1:
+ case A2:
+ case A3:
+ return true;
+ default:
+ return false;
+ }
+}
+
----------------
This is equivalent to GPRMM16RegClass.contains(Reg)
================
Comment at: lib/Target/Mips/MicroMips32SizeReduction.cpp:290-306
@@ +289,19 @@
+
+// Returns true if the register Reg is $0, $17, or $2-$7.
+static bool isMMSourceRegister(unsigned Reg) {
+ using namespace Mips;
+ switch (Reg) {
+ case ZERO:
+ case S1:
+ case V0:
+ case V1:
+ case A0:
+ case A1:
+ case A2:
+ case A3:
+ return true;
+ default:
+ return false;
+ }
+}
+// Returns true if the machine operand MO is register SP
----------------
Similarly, this is equivalent to GPRMM16ZeroRegClass.contains(Reg)
================
Comment at: lib/Target/Mips/MicroMips32SizeReduction.cpp:307-312
@@ +306,8 @@
+}
+// Returns true if the machine operand MO is register SP
+static bool IsSP(const MachineOperand &MO) {
+ if (MO.isReg() && (MO.getReg() == Mips::SP))
+ return true;
+ return false;
+}
+
----------------
This is only correct for the o32 and n32 ABIs. If you check for Mips::SP64 as well then it will cover the n64 ABI too.
================
Comment at: lib/Target/Mips/MicroMips32SizeReduction.cpp:343-344
@@ +342,4 @@
+static bool GetReg(MachineInstr *MI, unsigned op, unsigned ®) {
+ if (op >= MI->getNumOperands())
+ return false;
+ if (!MI->getOperand(op).isReg())
----------------
I'd expect this to be indicative of a bug somewhere else. Should it be an assertion?
================
Comment at: lib/Target/Mips/MicroMips32SizeReduction.cpp:353
@@ +352,3 @@
+// registers
+static bool EqualRegsInInstr(MachineInstr *MI, uint8_t Op1, uint8_t Op2) {
+ unsigned reg1, reg2;
----------------
Operand indices are 'unsigned' rather than uint8_t
================
Comment at: lib/Target/Mips/MicroMips32SizeReduction.cpp:354-361
@@ +353,10 @@
+static bool EqualRegsInInstr(MachineInstr *MI, uint8_t Op1, uint8_t Op2) {
+ unsigned reg1, reg2;
+ if (!GetReg(MI, Op1, reg1))
+ return false;
+ if (!GetReg(MI, Op2, reg2))
+ return false;
+ if (reg1 != reg2)
+ return false;
+ return true;
+}
----------------
Am I right in thinking this is to check the pointer registers of each instruction are the same? If so, this should be ok but the function name should indicate that it's only suitable for pointers.
If integers are a possibility then we will also need to handle the fact that V0 != V0_64 despite being the same register.
================
Comment at: lib/Target/Mips/MicroMips32SizeReduction.cpp:366-381
@@ +365,18 @@
+static bool ConsecutiveRegisters(unsigned Reg1, unsigned Reg2) {
+ static SmallVector<unsigned, 31> registers = {
+ Mips::AT, Mips::V0, Mips::V1, Mips::A0, Mips::A1, Mips::A2, Mips::A3,
+ Mips::T0, Mips::T1, Mips::T2, Mips::T3, Mips::T4, Mips::T5, Mips::T6,
+ Mips::T7, Mips::S0, Mips::S1, Mips::S2, Mips::S3, Mips::S4, Mips::S5,
+ Mips::S6, Mips::S7, Mips::T8, Mips::T9, Mips::K0, Mips::K1, Mips::GP,
+ Mips::SP, Mips::FP, Mips::RA};
+
+ for (uint8_t i = 0; i < registers.size() - 1; i++) {
+ if (registers[i] == Reg1) {
+ if (registers[i + 1] == Reg2)
+ return true;
+ else
+ return false;
+ }
+ }
+ return false;
+}
----------------
I haven't tested this but something like:
const auto &End = GPR32RegClass.end();
const auto &I = std::find(GPR32RegClass.begin(), End, Reg1);
if (I == End || *I != Reg1)
return false;
I++;
if (*I == Reg2)
return true;
return false;
should be the equivalent without duplicating our register classes.
We ought to account for the '*_64' versions of these registers too which can be handled using GPR64RegClass.
================
Comment at: lib/Target/Mips/MicroMips32SizeReduction.cpp:385-386
@@ +384,4 @@
+// Returns true if the variable value has the number of least-significant zero
+// bits equal to shift
+// and if the shifted value is between the bounds
+static bool InRange(int64_t value, unsigned short shift, int lbound,
----------------
Line wrapping
================
Comment at: lib/Target/Mips/MicroMips32SizeReduction.cpp:387-393
@@ +386,9 @@
+// and if the shifted value is between the bounds
+static bool InRange(int64_t value, unsigned short shift, int lbound,
+ int hbound) {
+ int64_t value2 = value >> shift;
+ if ((value2 << shift) == value && (value2 >= lbound) && (value2 < hbound))
+ return true;
+ return false;
+}
+
----------------
MathExtras.h has isShiftedInt() and isShiftedUInt() templates that are equivalent to this function
================
Comment at: lib/Target/Mips/MicroMips32SizeReduction.cpp:474-485
@@ +473,14 @@
+// of the registers reg1 and reg2
+static bool InstrUsesRegs(MachineInstr *MI, unsigned reg1, unsigned reg2) {
+ uint8_t numOp = MI->getNumOperands();
+ unsigned reg;
+ // Iterates through the registers that this instruction uses
+ for (uint8_t i = 0; i < numOp; ++i) {
+ if (GetReg(MI, i, reg)) {
+ if ((reg == reg1) || (reg == reg2))
+ return true;
+ }
+ }
+ return false;
+}
+
----------------
This is equivalent to this function:
MI->readsRegister(reg1) || MI->readsRegister(reg2)
If you pass the TRI argument then it will check for reads that occur because of super-register reads too. I don't think it can check for reads caused by sub-register reads though.
================
Comment at: lib/Target/Mips/MicroMips32SizeReduction.cpp:492
@@ +491,3 @@
+ unsigned reg;
+ for (uint8_t i = 0; i < numOp; ++i) {
+ if (GetReg(MI, i, reg))
----------------
We should use C++11's range based for loop
for (const auto &I : MI->operands())
================
Comment at: lib/Target/Mips/MicroMips32SizeReduction.cpp:507-508
@@ +506,4 @@
+
+ if (!GetImm(MI, 2, offset))
+ return false;
+
----------------
According to the tablegen definition, it's not guaranteed to be operand 2 when variable_ops for the Lwm/Swm is non-empty. It will be operand NumOps-1
================
Comment at: lib/Target/Mips/MicroMips32SizeReduction.cpp:516-517
@@ +515,4 @@
+
+ if (!GetReg(MI, 1, base))
+ return false;
+
----------------
Similarly, it's not guaranteed to be operand 1 when variable_ops for the Lwm/Swm is non-empty. It will be operand NumOps-2
================
Comment at: lib/Target/Mips/MicroMips32SizeReduction.cpp:532-533
@@ +531,4 @@
+ int64_t offset;
+ if (!GetImm(MI, 2, offset))
+ return false;
+
----------------
Likewise
================
Comment at: lib/Target/Mips/MicroMips32SizeReduction.cpp:539
@@ +538,3 @@
+ unsigned num = operands.size();
+ if (num < 2 || num > 5)
+ return false;
----------------
At minimum we have two sources/results ($16 and $31) along with a base address and offset so shouldn't the lower bound be 3.
Similarly: At most, we have five sources/results ($16-$19, and $31) along with the base address and offset so shouldn't the upper bound be 7?
================
Comment at: lib/Target/Mips/MicroMips32SizeReduction.cpp:551-560
@@ +550,12 @@
+static bool findLWMSWMPositon(unsigned reg, unsigned &Position) {
+ SmallVector<unsigned, 10> regs = {Mips::S0, Mips::S1, Mips::S2, Mips::S3,
+ Mips::S4, Mips::S5, Mips::S6, Mips::S7,
+ Mips::FP, Mips::RA};
+ for (uint8_t k = 0; k < regs.size(); k++)
+ if (reg == regs[k]) {
+ Position = k;
+ return true;
+ }
+
+ return false;
+}
----------------
std::find using GPR16MMRegClass and std::distance should be equivalent to this.
================
Comment at: lib/Target/Mips/MicroMips32SizeReduction.cpp:564
@@ +563,3 @@
+MicroMips32SizeReduce::MicroMips32SizeReduce() : MachineFunctionPass(ID) {
+ std::sort(ReduceTable.begin(), ReduceTable.end());
+}
----------------
Rather than sort at startup, can we just keep the table sorted and assert std::is_sorted()? If we do want to std::sort() then the best place to put it would be in tablegen when we start tablegen-erating the array.
================
Comment at: lib/Target/Mips/MicroMips32SizeReduction.cpp:760
@@ +759,3 @@
+
+ for (unsigned i = 0; i < instrs.size(); i++)
+ if (instrs[i].found)
----------------
Use range-based for loop
================
Comment at: lib/Target/Mips/MicroMips32SizeReduction.cpp:24
@@ +23,3 @@
+#include <algorithm>
+#include <vector>
+
----------------
Do we really need std::vector or can we use one of the alternatives from http://llvm.org/docs/ProgrammersManual.html#sequential-containers-std-vector-std-list-etc?
For example, I see there's a std::vector<MachineOperand> below. This would probably be better as a SmallVector<MachineOperand, 4> or similar.
================
Comment at: lib/Target/Mips/MicroMips32SizeReduction.cpp:37
@@ +36,3 @@
+// Order of operands
+enum opNum { NA, opAll, op01, op02, op12, op2, opLwpSwp };
+
----------------
What do these enumerators mean?
Naming nit: Types and enumerators should begin with a capital. They should also have a prefix such as 'ON_'. More information can be found at http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly
================
Comment at: lib/Target/Mips/MicroMips32SizeReduction.cpp:601
@@ +600,3 @@
+ const MachineBasicBlock::instr_iterator &E = fa.E;
+ MachineBasicBlock::instr_iterator &NNextMII = fa.NNextMII;
+ const ReduceEntry &Entry = fa.Entry;
----------------
Is the double-N meaninful?
================
Comment at: lib/Target/Mips/MicroMips32SizeReduction.cpp:604
@@ +603,3 @@
+
+ MachineBasicBlock::instr_iterator NextMII; //, StartMII = MII;
+
----------------
Please delete the commented out code.
================
Comment at: lib/Target/Mips/MicroMips32SizeReduction.cpp:940-941
@@ +939,4 @@
+ }
+ if (i < instrs.size() && instrs[9].found)
+ operands.push_back(instrs[9].MI->getOperand(0));
+
----------------
Could you add a comment explaining why instrs[9] is special? What does '9' correspond to?
================
Comment at: lib/Target/Mips/MicroMips32SizeReduction.cpp:1003-1052
@@ +1002,52 @@
+
+bool MicroMips32SizeReduce::ReplaceInstruction(MachineBasicBlock &MBB,
+ MachineInstr *MI,
+ const ReduceEntry &Entry) {
+
+ // Add the 16-bit instruction.
+ const MCInstrDesc &NewMCID = MipsII->get(Entry.NarrowOpc());
+ DebugLoc dl = MI->getDebugLoc();
+ MachineInstrBuilder MIB = BuildMI(MBB, MI, dl, NewMCID);
+
+ enum opNum opNums = Entry.TransferOperands();
+ if (opNums == op02) {
+ MIB.addOperand(MI->getOperand(0));
+ MIB.addOperand(MI->getOperand(2));
+ } else if (opNums == op01) {
+ MIB.addOperand(MI->getOperand(0));
+ MIB.addOperand(MI->getOperand(1));
+ } else if (opNums == op12) {
+ MIB.addOperand(MI->getOperand(1));
+ MIB.addOperand(MI->getOperand(2));
+ } else if (opNums == op2) {
+ MIB.addOperand(MI->getOperand(2));
+ } else if ((opNums == opAll) && Entry.SmallerNumRegs()) {
+ if (EqualRegsInInstr(MI, 0, 1)) {
+ MIB.addOperand(MI->getOperand(0));
+ MIB.addOperand(MI->getOperand(2));
+ MIB.addOperand(MI->getOperand(1));
+ } else {
+ MIB.addOperand(MI->getOperand(0));
+ MIB.addOperand(MI->getOperand(1));
+ MIB.addOperand(MI->getOperand(2));
+ }
+ } else if (opNums == opAll)
+ for (uint8_t i = 0, e = MI->getNumOperands(); i != e; ++i) {
+ const MachineOperand &MO = MI->getOperand(i);
+ MIB.addOperand(MO);
+ }
+ else
+ return false;
+
+ // Transfer memoperands.
+ MIB->setMemRefs(MI->memoperands_begin(), MI->memoperands_end());
+
+ // Transfer MI flags.
+ MIB.setMIFlags(MI->getFlags());
+
+ DEBUG(errs() << "Converted 32-bit: " << *MI << " to 16-bit: " << *MIB);
+ MBB.erase_instr(MI);
+ ++NumReduced;
+ return true;
+}
+
----------------
I think this is just mutating one MachineInst into another similar one. Do we really need to build a new instruction and transfer everything or can we just call MI->setDesc()?
================
Comment at: lib/Target/Mips/MicroMipsInstrInfo.td:529-555
@@ -528,29 +528,29 @@
InstrItinClass Itin = NoItinerary, ComplexPattern Addr = addr> :
- InstSE<(outs), (ins reglist:$rt, mem_mm_12:$addr),
+ InstSE<(outs), (ins reglist:$rt, variable_ops, mem_mm_12:$addr),
!strconcat(opstr, "\t$rt, $addr"), [], Itin, FrmI, opstr> {
let DecoderMethod = "DecodeMemMMImm12";
let mayStore = 1;
}
class LoadMultMM<string opstr,
InstrItinClass Itin = NoItinerary, ComplexPattern Addr = addr> :
- InstSE<(outs reglist:$rt), (ins mem_mm_12:$addr),
+ InstSE<(outs reglist:$rt, variable_ops), (ins mem_mm_12:$addr),
!strconcat(opstr, "\t$rt, $addr"), [], Itin, FrmI, opstr> {
let DecoderMethod = "DecodeMemMMImm12";
let mayLoad = 1;
}
class StoreMultMM16<string opstr,
InstrItinClass Itin = NoItinerary,
ComplexPattern Addr = addr> :
- MicroMipsInst16<(outs), (ins reglist16:$rt, mem_mm_4sp:$addr),
+ MicroMipsInst16<(outs), (ins reglist16:$rt, variable_ops, mem_mm_4sp:$addr),
!strconcat(opstr, "\t$rt, $addr"), [], Itin, FrmI> {
let DecoderMethod = "DecodeMemMMReglistImm4Lsl2";
let mayStore = 1;
}
class LoadMultMM16<string opstr,
InstrItinClass Itin = NoItinerary,
ComplexPattern Addr = addr> :
- MicroMipsInst16<(outs reglist16:$rt), (ins mem_mm_4sp:$addr),
+ MicroMipsInst16<(outs reglist16:$rt, variable_ops), (ins mem_mm_4sp:$addr),
!strconcat(opstr, "\t$rt, $addr"), [], Itin, FrmI> {
----------------
If we have explicit operands for the variable-length portion, do we still want the reglist16 operands? I believe the variable length portion covers the same operands as the reglist16's.
================
Comment at: test/CodeGen/Mips/micromips-lwm-swm-lwp-swp-sw16.ll:1
@@ +1,2 @@
+; RUN: llc -march=mipsel -mattr=+micromips < %s | FileCheck %s
+
----------------
(filename) Could you move this into a subdirectory for testing this pass? I'm thinking that the number of tests is going to grow over time and we ought to make it easy to tell which tests cover this pass.
================
Comment at: test/CodeGen/Mips/micromips-lwsp-swsp.ll:1
@@ +1,2 @@
+; RUN: llc -march=mipsel -mattr=+micromips -filetype=asm -asm-show-inst < %s | FileCheck %s
+
----------------
(filename) Could you move this into a subdirectory for testing this pass?
http://reviews.llvm.org/D15144
More information about the llvm-commits
mailing list