[ARM] Merge duplicated Thumb2SizeReduce code (NFC)

[ksyx]

Summary

Merge duplicated code into procedure for code cleanness and readability and use llvm::dbgs() instead of errs() for consistency with other parts.

[ksyx]

Apply suggestions from clangfmt

[efriedma]

I don't think the setMIFlags() call belongs in the helper; it's technically duplicated across all the functions, but it's conceptually part of creating an equivalent instruction, and the steps for that are different in each place.

I don't think refactoring the debug counter increment is helpful; counters aren't normally passed around that way, so it's a bit confusing to follow.

At that point, the helper is basically three lines, so I'm not sure the refactoring is worthwhile. Maybe if you wanted to add something else to the refactored function, but I'm not sure what that would be.

llvm/lib/Target/ARM/Thumb2SizeReduction.cpp

413

"llvm::" prefix isn't necessary; this file is "using namespace llvm".

[ksyx]

Thanks for the review and it sounds a good rationale. I am also wondering whether only changing errs to dbgs on the original code without changing other parts or using helper function a good change for consistency with other code?

[efriedma]

Yes, we should do that for consistency.

[ksyx]

Ok then I would only do that.

llvm/lib/Target/ARM/Thumb2SizeReduction.cpp

private:
     DenseMap<unsigned, unsigned> ReduceOpcodeMap;
 
     bool canAddPseudoFlagDep(MachineInstr *Use, bool IsSelfLoop);
 
     bool VerifyPredAndCC(MachineInstr *MI, const ReduceEntry &Entry,
                          bool is2Addr, ARMCC::CondCodes Pred,
                          bool LiveCPSR, bool &HasCC, bool &CCDead);
 
+    void PerformReduce(MachineBasicBlock &MBB, MachineInstr *MI,
+                       MachineInstrBuilder &MIB, llvm::Statistic &Counter);
+
     bool ReduceLoadStore(MachineBasicBlock &MBB, MachineInstr *MI,
                          const ReduceEntry &Entry);
 
     bool ReduceSpecial(MachineBasicBlock &MBB, MachineInstr *MI,
                        const ReduceEntry &Entry, bool LiveCPSR, bool IsSelfLoop);
 
     /// ReduceTo2Addr - Reduce a 32-bit instruction to a 16-bit two-address
     /// instruction.
if (Reg == ARM::SP) {
         continue;
     }
     if (!isARMLowRegister(Reg))
       return false;
   }
   return true;
 }
 
+
+void
+Thumb2SizeReduce::PerformReduce(MachineBasicBlock &MBB, MachineInstr *MI,
+                                MachineInstrBuilder &MIB,
+                                llvm::Statistic &Counter) {
+  // Transfer MI flags.
+  MIB.setMIFlags(MI->getFlags());
+
+  LLVM_DEBUG(llvm::dbgs() << "Converted 32-bit: " << *MI
+                          << "       to 16-bit: " << *MIB);
+
+  MBB.erase_instr(MI);
+  ++Counter;
+}
+
 bool
 Thumb2SizeReduce::ReduceLoadStore(MachineBasicBlock &MBB, MachineInstr *MI,
                                   const ReduceEntry &Entry) {
   if (ReduceLimitLdSt != -1 && ((int)NumLdSts >= ReduceLimitLdSt))
     return false;
 
   unsigned Scale = 1;
   bool HasImmOffset = false;
Thumb2SizeReduce::ReduceLoadStore(MachineBasicBlock &MBB, MachineInstr *MI,
 
   // Transfer the rest of operands.
   for (const MachineOperand &MO : llvm::drop_begin(MI->operands(), OpNum))
     MIB.add(MO);
 
   // Transfer memoperands.
   MIB.setMemRefs(MI->memoperands());
 
-  // Transfer MI flags.
-  MIB.setMIFlags(MI->getFlags());
-
-  LLVM_DEBUG(errs() << "Converted 32-bit: " << *MI
-                    << "       to 16-bit: " << *MIB);
-
-  MBB.erase_instr(MI);
-  ++NumLdSts;
+  PerformReduce(MBB, MI, MIB, NumLdSts);
   return true;
 }
 
 bool
 Thumb2SizeReduce::ReduceSpecial(MachineBasicBlock &MBB, MachineInstr *MI,
                                 const ReduceEntry &Entry,
                                 bool LiveCPSR, bool IsSelfLoop) {
   unsigned Opc = MI->getOpcode();
if (Opc == ARM::t2ADDri) {
     MachineInstrBuilder MIB =
         BuildMI(MBB, MI, MI->getDebugLoc(),
                 TII->get(ARM::tADDrSPi))
             .add(MI->getOperand(0))
             .add(MI->getOperand(1))
             .addImm(Imm / 4) // The tADDrSPi has an implied scale by four.
             .add(predOps(ARMCC::AL));
 
-    // Transfer MI flags.
-    MIB.setMIFlags(MI->getFlags());
-
-    LLVM_DEBUG(errs() << "Converted 32-bit: " << *MI
-                      << "       to 16-bit: " << *MIB);
-
-    MBB.erase_instr(MI);
-    ++NumNarrows;
+    PerformReduce(MBB, MI, MIB, NumNarrows);
     return true;
   }
 
   if (Entry.LowRegs1 && !VerifyLowRegs(MI))
     return false;
 
   if (MI->mayLoadOrStore())
     return ReduceLoadStore(MBB, MI, Entry);
Thumb2SizeReduce::ReduceTo2Addr(MachineBasicBlock &MBB, MachineInstr *MI,
   for (unsigned i = 1, e = MI->getNumOperands(); i != e; ++i) {
     if (i < NumOps && MCID.OpInfo[i].isOptionalDef())
       continue;
     if (SkipPred && MCID.OpInfo[i].isPredicate())
       continue;
     MIB.add(MI->getOperand(i));
   }
 
-  // Transfer MI flags.
-  MIB.setMIFlags(MI->getFlags());
-
-  LLVM_DEBUG(errs() << "Converted 32-bit: " << *MI
-                    << "       to 16-bit: " << *MIB);
-
-  MBB.erase_instr(MI);
-  ++Num2Addrs;
+  PerformReduce(MBB, MI, MIB, Num2Addrs);
   return true;
 }
 
 bool
 Thumb2SizeReduce::ReduceToNarrow(MachineBasicBlock &MBB, MachineInstr *MI,
                                  const ReduceEntry &Entry,
                                  bool LiveCPSR, bool IsSelfLoop) {
   if (ReduceLimit != -1 && ((int)NumNarrows >= ReduceLimit))
if (MO.isReg() && MO.isImplicit() && MO.getReg() == ARM::CPSR)
       // Skip implicit def of CPSR. Either it's modeled as an optional
       // def now or it's already an implicit def on the new instruction.
       continue;
     MIB.add(MO);
   }
   if (!MCID.isPredicable() && NewMCID.isPredicable())
     MIB.add(predOps(ARMCC::AL));
 
-  // Transfer MI flags.
-  MIB.setMIFlags(MI->getFlags());
-
-  LLVM_DEBUG(errs() << "Converted 32-bit: " << *MI
-                    << "       to 16-bit: " << *MIB);
-
-  MBB.erase_instr(MI);
-  ++NumNarrows;
+  PerformReduce(MBB, MI, MIB, NumNarrows);
   return true;
 }
 
 static bool UpdateCPSRDef(MachineInstr &MI, bool LiveCPSR, bool &DefCPSR) {
   bool HasDef = false;
   for (const MachineOperand &MO : MI.operands()) {
     if (!MO.isReg() || MO.isUndef() || MO.isUse())
       continue;