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

[Random06457]

Summary

Early revisions of the VR4300 have a hardware bug where two consecutive multiplications can produce an incorrect result in the second multiply.
This revision adds the -mfix4300 flag to llvm (and clang) which, when passed, provides a software fix for this issue.

More precise description of the "mulmul" bug:

1: mul.[s,d] fd,fs,ft
2: mul.[s,d] fd,fs,ft  or  [D]MULT[U] rs,rt

When the above sequence is executed by the CPU, if at least one of the source operands of the first mul instruction happens to be sNaN, 0 or Infinity, then the second mul instruction may produce an incorrect result.
This can happen both if the two mul instructions are next to each other and if the first one is in a delay slot and the second is the first instruction of the branch target.

Description of the fix:
This fix adds a backend pass to llvm which scans for mul instructions in each basic block and inserts a nop whenever the following conditions are met:

• The current instruction is a single or double-precision floating-point mul instruction.
• The next instruction is either a mul instruction (any kind) or a branch instruction.

Note:
I chose -mfix4300 as a name for the flag to follow the GCC nomenclature but I don't know if this is a good name.

[atanasyan]

Thanks for the patch. Some notes are below.

llvm/lib/Target/Mips/MipsMulMulBugPass.cpp

17

We can move this option to the MipsTargetMachine.cpp and just do not add the pass when it is not necessary.

20

Put this class into an anonymous namespace to reduce work for a linker.

32

Let's rename the function to the fixMulMulBB and move it to the private section of the class.

37

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.

38

Do you really need to keep a pointer to the Subtarget in the object?

49–50

These lines can be merged into the single one:

MipsII = MF.getSubtarget<MipsSubtarget>().getInstrInfo();

53–56

This code can be made a bit more compact:

for (auto &MBB: MF)
  Modified |= FixMulMulBB(MBB);

61

This function does not work with null pointer so change the argument's type to a reference.

74

Ditto

96–104

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);
...

111

You do not need a new MBB variable. Use MBB passed as an argument to the FixMulMulBB.

[Random06457]

Addressed the comments.
I also updated isFirstMul to exclude integer multiplications which do not produce the bug. That change made me fix the tests too.

[atanasyan]

LGTM. Thanks for the patch.

clang/include/clang/Driver/Options.td

 def mxgot : Flag<["-"], "mxgot">, Group<m_mips_Features_Group>;
 def mno_xgot : Flag<["-"], "mno-xgot">, Group<m_mips_Features_Group>;
 def mldc1_sdc1 : Flag<["-"], "mldc1-sdc1">, Group<m_mips_Features_Group>;
 def mno_ldc1_sdc1 : Flag<["-"], "mno-ldc1-sdc1">, Group<m_mips_Features_Group>;
 def mcheck_zero_division : Flag<["-"], "mcheck-zero-division">,
                            Group<m_mips_Features_Group>;
 def mno_check_zero_division : Flag<["-"], "mno-check-zero-division">,
                               Group<m_mips_Features_Group>;
+def mfix4300 : Flag<["-"], "mfix4300">, Group<m_mips_Features_Group>;
 def mcompact_branches_EQ : Joined<["-"], "mcompact-branches=">,
                            Group<m_mips_Features_Group>;
 def mbranch_likely : Flag<["-"], "mbranch-likely">, Group<m_Group>,
   IgnoredGCCCompat;
 def mno_branch_likely : Flag<["-"], "mno-branch-likely">, Group<m_Group>,
   IgnoredGCCCompat;
 def mindirect_jump_EQ : Joined<["-"], "mindirect-jump=">,
   Group<m_mips_Features_Group>,

clang/lib/Driver/ToolChains/Clang.cpp

void Clang::AddMIPSTargetArgs(const ArgList &Args,
   if (Arg *A = Args.getLastArg(options::OPT_mcheck_zero_division,
                                options::OPT_mno_check_zero_division)) {
     if (A->getOption().matches(options::OPT_mno_check_zero_division)) {
       CmdArgs.push_back("-mllvm");
       CmdArgs.push_back("-mno-check-zero-division");
     }
   }
 
+  if (Arg *A = Args.getLastArg(options::OPT_mfix4300)) {
+    CmdArgs.push_back("-mllvm");
+    CmdArgs.push_back("-mfix4300");
+  }
+
   if (Arg *A = Args.getLastArg(options::OPT_G)) {
     StringRef v = A->getValue();
     CmdArgs.push_back("-mllvm");
     CmdArgs.push_back(Args.MakeArgString("-mips-ssection-threshold=" + v));
     A->claim();
   }
 
   Arg *GPOpt = Args.getLastArg(options::OPT_mgpopt, options::OPT_mno_gpopt);

llvm/lib/Target/Mips/CMakeLists.txt

add_llvm_target(MipsCodeGen
   MipsSEInstrInfo.cpp
   MipsSEISelDAGToDAG.cpp
   MipsSEISelLowering.cpp
   MipsSERegisterInfo.cpp
   MipsSubtarget.cpp
   MipsTargetMachine.cpp
   MipsTargetObjectFile.cpp
   MicroMipsSizeReduction.cpp
+  MipsMulMulBugPass.cpp
 
   LINK_COMPONENTS
   Analysis
   AsmPrinter
   CodeGen
   Core
   MC
   MipsDesc

llvm/lib/Target/Mips/Mips.h

namespace llvm {
   FunctionPass *createMipsModuleISelDagPass();
   FunctionPass *createMipsOptimizePICCallPass();
   FunctionPass *createMipsDelaySlotFillerPass();
   FunctionPass *createMipsBranchExpansion();
   FunctionPass *createMipsConstantIslandPass();
   FunctionPass *createMicroMipsSizeReducePass();
   FunctionPass *createMipsExpandPseudoPass();
   FunctionPass *createMipsPreLegalizeCombiner();
+  FunctionPass *createMipsMulMulBugPass();
 
   InstructionSelector *createMipsInstructionSelector(const MipsTargetMachine &,
                                                      MipsSubtarget &,
                                                      MipsRegisterBankInfo &);
 
   void initializeMipsDelaySlotFillerPass(PassRegistry &);
   void initializeMipsBranchExpansionPass(PassRegistry &);
   void initializeMicroMipsSizeReducePass(PassRegistry &);
   void initializeMipsPreLegalizerCombinerPass(PassRegistry&);
+  void initializeMipsMulMulBugFixPass(PassRegistry&);
 } // end namespace llvm;
 
 #endif

llvm/lib/Target/Mips/MipsMulMulBugPass.cpp

+//===- MipsMulMulBugPass.cpp - Mips VR4300 mulmul bugfix pass -------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// Early revisions of the VR4300 have a hardware bug where two consecutive
+// multiplications can produce an incorrect result in the second multiply.
+//
+// This pass scans for mul instructions in each basic block and inserts
+// a nop whenever the following conditions are met:
+//
+// - The current instruction is a single or double-precision floating-point
+//   mul instruction.
+// - The next instruction is either a mul instruction (any kind)
+//   or a branch instruction.
+//===----------------------------------------------------------------------===//
+
+#include "Mips.h"
+#include "MipsInstrInfo.h"
+#include "MipsSubtarget.h"
+#include "llvm/CodeGen/MachineBasicBlock.h"
+#include "llvm/CodeGen/MachineFunction.h"
+#include "llvm/CodeGen/MachineFunctionPass.h"
+#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/Debug.h"
+#include "llvm/Target/TargetMachine.h"
+
+#define DEBUG_TYPE "mips-vr4300-mulmul-fix"
+
+using namespace llvm;
+
+namespace {
+
+class MipsMulMulBugFix : public MachineFunctionPass {
+public:
+  MipsMulMulBugFix() : MachineFunctionPass(ID) {
+    initializeMipsMulMulBugFixPass(*PassRegistry::getPassRegistry());
+  }
+
+  StringRef getPassName() const override { return "Mips VR4300 mulmul bugfix"; }
+
+  MachineFunctionProperties getRequiredProperties() const override {
+    return MachineFunctionProperties().set(
+        MachineFunctionProperties::Property::NoVRegs);
+  }
+
+  bool runOnMachineFunction(MachineFunction &MF) override;
+
+  static char ID;
+
+private:
+  bool fixMulMulBB(MachineBasicBlock &MBB, const MipsInstrInfo &MipsII);
+};
+
+} // namespace
+
+INITIALIZE_PASS(MipsMulMulBugFix, "mips-vr4300-mulmul-fix",
+                "Mips VR4300 mulmul bugfix", false, false)
+
+char MipsMulMulBugFix::ID = 0;
+
+bool MipsMulMulBugFix::runOnMachineFunction(MachineFunction &MF) {
+  const MipsInstrInfo &MipsII =
+      *static_cast<const MipsInstrInfo *>(MF.getSubtarget().getInstrInfo());
+
+  bool Modified = false;
+
+  for (auto &MBB : MF)
+    Modified |= fixMulMulBB(MBB, MipsII);
+
+  return Modified;
+}
+
+static bool isFirstMul(const MachineInstr &MI) {
+  switch (MI.getOpcode()) {
+  case Mips::FMUL_S:
+  case Mips::FMUL_D:
+  case Mips::FMUL_D32:
+  case Mips::FMUL_D64:
+    return true;
+  default:
+    return false;
+  }
+}
+
+static bool isSecondMulOrBranch(const MachineInstr &MI) {
+  if (MI.isBranch() || MI.isIndirectBranch() || MI.isCall())
+    return true;
+
+  switch (MI.getOpcode()) {
+  case Mips::MUL:
+  case Mips::FMUL_S:
+  case Mips::FMUL_D:
+  case Mips::FMUL_D32:
+  case Mips::FMUL_D64:
+  case Mips::MULT:
+  case Mips::MULTu:
+  case Mips::DMULT:
+  case Mips::DMULTu:
+    return true;
+  default:
+    return false;
+  }
+}
+
+bool MipsMulMulBugFix::fixMulMulBB(MachineBasicBlock &MBB,
+                                   const MipsInstrInfo &MipsII) {
+  bool Modified = false;
+
+  // Iterate through the instructions in the basic block
+  for (MachineBasicBlock::instr_iterator MII = MBB.instr_begin(),
+                                         E = MBB.instr_end();
+       MII != E; ++MII) {
+
+    MachineBasicBlock::instr_iterator NextMII = std::next(MII);
+
+    // Trigger when the current instruction is a mul and the next instruction
+    // is either a mul or a branch in case the branch target start with a mul
+    if (NextMII != E && isFirstMul(*MII) && isSecondMulOrBranch(*NextMII)) {
+      LLVM_DEBUG(dbgs() << "Found mulmul!");
+
+      const MCInstrDesc &NewMCID = MipsII.get(Mips::NOP);
+      BuildMI(MBB, NextMII, DebugLoc(), NewMCID);
+      Modified = true;
+    }
+  }
+
+  return Modified;
+}
+
+FunctionPass *llvm::createMipsMulMulBugPass() { return new MipsMulMulBugFix(); }

llvm/lib/Target/Mips/MipsTargetMachine.cpp

 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Target/TargetOptions.h"
 #include <string>
 
 using namespace llvm;
 
 #define DEBUG_TYPE "mips"
 
+static cl::opt<bool>
+    EnableMulMulFix("mfix4300", cl::init(false),
+                    cl::desc("Enable the VR4300 mulmul bug fix."), cl::Hidden);
+
 extern "C" LLVM_EXTERNAL_VISIBILITY void LLVMInitializeMipsTarget() {
   // Register the target.
   RegisterTargetMachine<MipsebTargetMachine> X(getTheMipsTarget());
   RegisterTargetMachine<MipselTargetMachine> Y(getTheMipselTarget());
   RegisterTargetMachine<MipsebTargetMachine> A(getTheMips64Target());
   RegisterTargetMachine<MipselTargetMachine> B(getTheMips64elTarget());
 
   PassRegistry *PR = PassRegistry::getPassRegistry();
   initializeGlobalISel(*PR);
   initializeMipsDelaySlotFillerPass(*PR);
   initializeMipsBranchExpansionPass(*PR);
   initializeMicroMipsSizeReducePass(*PR);
   initializeMipsPreLegalizerCombinerPass(*PR);
+  initializeMipsMulMulBugFixPass(*PR);
 }
 
 static std::string computeDataLayout(const Triple &TT, StringRef CPU,
                                      const TargetOptions &Options,
                                      bool isLittle) {
   std::string Ret;
   MipsABIInfo ABI = MipsABIInfo::computeTargetABI(TT, CPU, Options.MCOptions);
 

 void MipsPassConfig::addPreEmitPass() {
   // Expand pseudo instructions that are sensitive to register allocation.
   addPass(createMipsExpandPseudoPass());
 
   // The microMIPS size reduction pass performs instruction reselection for
   // instructions which can be remapped to a 16 bit instruction.
   addPass(createMicroMipsSizeReducePass());
 
+  // This pass inserts a nop instruction between two back-to-back multiplication
+  // instructions when the "mfix4300" flag is passed.
+  if (EnableMulMulFix)
+    addPass(createMipsMulMulBugPass());
+
   // The delay slot filler pass can potientially create forbidden slot hazards
   // for MIPSR6 and therefore it should go before MipsBranchExpansion pass.
   addPass(createMipsDelaySlotFillerPass());
 
   // This pass expands branches and takes care about the forbidden slot hazards.
   // Expanding branches may potentially create forbidden slot hazards for
   // MIPSR6, and fixing such hazard may potentially break a branch by extending
   // its offset out of range. That's why this pass combine these two tasks, and

llvm/test/CodeGen/Mips/vr4300-mulbranch.ll

+; RUN: llc -march=mips -mfix4300 -verify-machineinstrs < %s | FileCheck %s
+
+; Function Attrs: nounwind
+define dso_local void @fun_s(float %a) local_unnamed_addr #0 {
+entry:
+; CHECK-LABEL: fun_s
+; CHECK: mul.s
+; CHECK-NEXT: nop
+  %mul = fmul float %a, %a
+  tail call void @foo_s(float %mul) #2
+  ret void
+}
+
+declare dso_local void @foo_s(float) local_unnamed_addr #1
+
+; Function Attrs: nounwind
+define dso_local void @fun_d(double %a) local_unnamed_addr #0 {
+entry:
+; CHECK-LABEL: fun_d
+; CHECK: mul.d
+; CHECK-NEXT: nop
+  %mul = fmul double %a, %a
+  tail call void @foo_d(double %mul) #2
+  ret void
+}
+
+declare dso_local void @foo_d(double) local_unnamed_addr #1

llvm/test/CodeGen/Mips/vr4300-mulmul.ll

+; RUN: llc -march=mips -mfix4300 -verify-machineinstrs < %s | FileCheck %s
+
+; Function Attrs: mustprogress nofree norecurse nosync nounwind readnone willreturn
+define dso_local float @fun_s(float %x) local_unnamed_addr #0 {
+entry:
+; CHECK-LABEL: fun_s
+; CHECK: mul.s
+; CHECK-NEXT: nop
+; CHECK: mul.s
+  %mul = fmul float %x, %x
+  %mul1 = fmul float %mul, %x
+  ret float %mul1
+}
+
+define dso_local double @fun_d(double %x) local_unnamed_addr #0 {
+entry:
+; CHECK-LABEL: fun_d
+; CHECK: mul.d
+; CHECK-NEXT: nop
+; CHECK: mul.d
+  %mul = fmul double %x, %x
+  %mul1 = fmul double %mul, %x
+  ret double %mul1
+}