[PATCH] D116238: [mips] Add -mfix4300 flag to enable vr4300 mulmul bugfix pass

[Simon Atanasyan via Phabricator via llvm-commits]

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

Thanks for the patch. Some notes are below.



================
Comment at: llvm/lib/Target/Mips/MipsMulMulBugPass.cpp:16
+static cl::opt<bool>
+    EnableMulMulFix("mfix4300", cl::init(false),
+                    cl::desc("Enable the VR4300 mulmul bug fix."), cl::Hidden);
----------------
We can move this option to the `MipsTargetMachine.cpp` and just do not add the pass when it is not necessary.


================
Comment at: llvm/lib/Target/Mips/MipsMulMulBugPass.cpp:19
+
+class MipsMulMulBugFix : public MachineFunctionPass {
+public:
----------------
Put this class into an anonymous namespace to reduce work for a linker.


================
Comment at: llvm/lib/Target/Mips/MipsMulMulBugPass.cpp:31
+  bool runOnMachineFunction(MachineFunction &MF) override;
+  bool FixMulMulBB(MachineBasicBlock &MBB);
+
----------------
Let's rename the function to the `fixMulMulBB` and move it to the `private` section of the class.


================
Comment at: llvm/lib/Target/Mips/MipsMulMulBugPass.cpp:36
+private:
+  static const MipsInstrInfo *MipsII;
+  const MipsSubtarget *Subtarget;
----------------
I do not think it's a good idea to save `MipsInstrInfo` into the **static** field. AFAIK now passes cannot be run in parallel. But if that changes in the future we get a problem with the static field. As to me I would get a reference to the `MipsInstrInfo` in the `runOnMachineFunction` and pass this reference to the `FixMulMulBB` as a parameter.


================
Comment at: llvm/lib/Target/Mips/MipsMulMulBugPass.cpp:37
+  static const MipsInstrInfo *MipsII;
+  const MipsSubtarget *Subtarget;
+};
----------------
Do you really need to keep a pointer to the `Subtarget` in the object?


================
Comment at: llvm/lib/Target/Mips/MipsMulMulBugPass.cpp:48-49
+
+  Subtarget = &static_cast<const MipsSubtarget &>(MF.getSubtarget());
+  MipsII = static_cast<const MipsInstrInfo *>(Subtarget->getInstrInfo());
+
----------------
These lines can be merged into the single one:
```
MipsII = MF.getSubtarget<MipsSubtarget>().getInstrInfo();
```


================
Comment at: llvm/lib/Target/Mips/MipsMulMulBugPass.cpp:52-55
+  MachineFunction::iterator I = MF.begin(), E = MF.end();
+
+  for (; I != E; ++I)
+    Modified |= FixMulMulBB(*I);
----------------
This code can be made a bit more compact:
```
for (auto &MBB: MF)
  Modified |= FixMulMulBB(MBB);
```


================
Comment at: llvm/lib/Target/Mips/MipsMulMulBugPass.cpp:60
+
+static bool isFirstMul(const MachineInstr *MI) {
+  switch (MI->getOpcode()) {
----------------
This function does not work with null pointer so change the argument's type to a reference.


================
Comment at: llvm/lib/Target/Mips/MipsMulMulBugPass.cpp:73
+
+static bool isSecondMulOrBranch(const MachineInstr *MI) {
+  if (MI->isBranch() || MI->isIndirectBranch() || MI->isCall())
----------------
Ditto


================
Comment at: llvm/lib/Target/Mips/MipsMulMulBugPass.cpp:95-103
+  MachineBasicBlock::instr_iterator MII = MBB.instr_begin(),
+                                    E = MBB.instr_end();
+  MachineBasicBlock::instr_iterator NextMII;
+
+  // Iterate through the instructions in the basic block
+  for (; MII != E; MII = NextMII) {
+
----------------
`std::next` call and the iterator incrementation are cheap calls. So we can write the loop in a more idiomatic form:
```
for (MachineBasicBlock::instr_iterator MII = MBB.instr_begin(),
                                       E = MBB.instr_end();
     MII != E; ++MII) {
  MachineBasicBlock::instr_iterator NextMII = std::next(MII);
...
```


================
Comment at: llvm/lib/Target/Mips/MipsMulMulBugPass.cpp:110
+
+      MachineBasicBlock &MBB = *MI->getParent();
+      const MCInstrDesc &NewMCID = MipsII->get(Mips::NOP);
----------------
You do not need a new `MBB` variable. Use `MBB` passed as an argument to the `FixMulMulBB`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116238