[PATCH] D115208: [NFC][MachineInstr] No need to std::move the DebugLoc

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In D115208#3175304 <https://reviews.llvm.org/D115208#3175304>, @mtrofin wrote:

> In D115208#3175303 <https://reviews.llvm.org/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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115208