[NFC][MachineInstr] Pass-by-value DebugLoc in CreateMachineInstr

[mtrofin]

Summary

DebugLoc is cheap to move, passing it by-val rather than const ref to take advantage of the fact that it is consumed that way by the MachineInstr ctor, which creates some optimization oportunities.

[dblaikie]

I /think/ given that DebugLoc has a TrackingMDNodeRef member, which has a TrackingMDRef member which has cheaper move than copy, then DebugLoc /probably/ should be passed by value when a copy is going to be made - since it means callers can pass by lvalue or rvalue and get move opportunities when possible.

Given this API scenario:

CheapToMove Dest;
void Caller() {
  CheapToMove Source;
  Intermediate(Source); // 1 
  Intermediate(CheapToMove()); //2
}
void Implementation(/* What Type Goes here? */ CTM) {
  Dest = std::move(CTM); // move is a no-op here if CTM hapens to be a const ref
}

If the type is const ref, then (1) produces a single copy and (2) produces a single copy.
If the type is by-value, then (1) produces a move and a copy (worse by a copy, that should be cheap), and (2) produces 2 moves (better for cheaply movable things).

Does that make sense?

[mtrofin]

In D115208#3175303, @dblaikie wrote:

I /think/ given that DebugLoc has a TrackingMDNodeRef member, which has a TrackingMDRef member which has cheaper move than copy, then DebugLoc /probably/ should be passed by value when a copy is going to be made - since it means callers can pass by lvalue or rvalue and get move opportunities when possible.

Given this API scenario:

CheapToMove Dest;
void Caller() {
  CheapToMove Source;
  Intermediate(Source); // 1 
  Intermediate(CheapToMove()); //2
}
void Implementation(/* What Type Goes here? */ CTM) {
  Dest = std::move(CTM); // move is a no-op here if CTM hapens to be a const ref
}

If the type is const ref, then (1) produces a single copy and (2) produces a single copy.
If the type is by-value, then (1) produces a move and a copy (worse by a copy, that should be cheap), and (2) produces 2 moves (better for cheaply movable things).

Does that make sense?

Ah, so you're saying MachineFunction::CreateMachineInstr should instead accept its DebugLoc by value - leaving the rest unchanged (i.e. revert this patch and change CreateMachineInstr instead)?

[dblaikie]

In D115208#3175304, @mtrofin wrote:

In D115208#3175303, @dblaikie wrote:

I /think/ given that DebugLoc has a TrackingMDNodeRef member, which has a TrackingMDRef member which has cheaper move than copy, then DebugLoc /probably/ should be passed by value when a copy is going to be made - since it means callers can pass by lvalue or rvalue and get move opportunities when possible.

Given this API scenario:

CheapToMove Dest;
void Caller() {
  CheapToMove Source;
  Intermediate(Source); // 1 
  Intermediate(CheapToMove()); //2
}
void Implementation(/* What Type Goes here? */ CTM) {
  Dest = std::move(CTM); // move is a no-op here if CTM hapens to be a const ref
}

If the type is const ref, then (1) produces a single copy and (2) produces a single copy.
If the type is by-value, then (1) produces a move and a copy (worse by a copy, that should be cheap), and (2) produces 2 moves (better for cheaply movable things).

Does that make sense?

Ah, so you're saying MachineFunction::CreateMachineInstr should instead accept its DebugLoc by value - leaving the rest unchanged (i.e. revert this patch and change CreateMachineInstr instead)?

Yeah, that sounds like the right direction - MachineFunction::CreateMachineInstr would take DebugLoc by value, and std::move it along to the MachineInstr constructor. DebugLoc used to be cheap to copy, I think, so there's probably lots of stuff in the codebase that passes it around by value without std::move, or other things - it's probably not the biggest perf issue to worry about in terms of doing widescale cleanup, but if you're visiting/want to clean something up, cleaning it up in this direction (pass by value+std::move if "consuming"/copying into a non-local-lifetime variable like a member in this case, etc).

[mtrofin]

feedback

[dblaikie]

looks good!

llvm/include/llvm/CodeGen/MachineFunction.h

public:
   /// implementation.
   void removeFromMBBNumbering(unsigned N) {
     assert(N < MBBNumbering.size() && "Illegal basic block #");
     MBBNumbering[N] = nullptr;
   }
 
   /// CreateMachineInstr - Allocate a new MachineInstr. Use this instead
   /// of `new MachineInstr'.
-  MachineInstr *CreateMachineInstr(const MCInstrDesc &MCID, const DebugLoc &DL,
+  MachineInstr *CreateMachineInstr(const MCInstrDesc &MCID, DebugLoc DL,
                                    bool NoImplicit = false);
 
   /// Create a new MachineInstr which is a copy of \p Orig, identical in all
   /// ways except the instruction has no parent, prev, or next. Bundling flags
   /// are reset.
   ///
   /// Note: Clones a single instruction, not whole instruction bundles.
   /// Does not perform target specific adjustments; consider using

llvm/lib/CodeGen/MachineFunction.cpp

for (auto MBBI = std::next(begin()), E = end(); MBBI != E; ++MBBI) {
     std::prev(MBBI)->setIsEndSection();
     CurrentSectionID = MBBI->getSectionID();
   }
   back().setIsEndSection();
 }
 
 /// Allocate a new MachineInstr. Use this instead of `new MachineInstr'.
 MachineInstr *MachineFunction::CreateMachineInstr(const MCInstrDesc &MCID,
-                                                  const DebugLoc &DL,
+                                                  DebugLoc DL,
                                                   bool NoImplicit) {
   return new (InstructionRecycler.Allocate<MachineInstr>(Allocator))
-      MachineInstr(*this, MCID, DL, NoImplicit);
+      MachineInstr(*this, MCID, std::move(DL), NoImplicit);
 }
 
 /// Create a new MachineInstr which is a copy of the 'Orig' instruction,
 /// identical in all ways except the instruction has no parent, prev, or next.
 MachineInstr *
 MachineFunction::CloneMachineInstr(const MachineInstr *Orig) {
   return new (InstructionRecycler.Allocate<MachineInstr>(Allocator))
              MachineInstr(*this, *Orig);