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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
