Assigning to a local object in a return statement prevents copy elision. NFC.

[• Quuxplusone]

Summary

I added a diagnostic along the lines of -Wpessimizing-move to detect return x = y suppressing copy elision, but I don't know if the diagnostic is really worth it. Anyway, here are the places where my diagnostic reported that copy elision would have been possible if not for the assignment.

P1155R1 in the post-San-Diego WG21 (C++ committee) mailing discusses whether WG21 should fix this pitfall by just changing the core language to permit copy elision in cases like these.

(Kona update: The bulk of P1155 is proceeding to CWG review, but specifically *not* the parts that explored the notion of permitting copy-elision in these specific cases.)

[dblaikie]

include/llvm/Support/BranchProbability.h

131–133 ↗

(On Diff #175193)

Would an alternative fix for this be to write ref-qualified overloads of opX=, then use them like:

return BranchProbability(*this) += RHS;

? (or, I suppose, "BranchProbability Prob(*this); return std::move(Prob) += RHS;")

lib/Target/AArch64/AArch64ISelLowering.cpp

7845 ↗

(On Diff #175193)

This looks like a good change regardless - the original phrasing is pretty weird, given that Cmp is going out of scope anyway, it could've been written as "return DAG.getNOT(...);" & skipped the assignment, but your solution here's more consistent with the surrounding code, I think

[• Quuxplusone]

include/llvm/Support/BranchProbability.h

131–133 ↗

(On Diff #175193)

Yes, absolutely that would work; but it would be horribly un-idiomatic.
Also, some wise people say that every assignment operator should ideally be lvalue-ref-qualified, and you're [hypothetically] suggesting to add rvalue-ref-qualified versions purely to enable an un-idiomatic style... I don't think it would be a good idea.

lib/Target/AArch64/AArch64ISelLowering.cpp

7845 ↗

(On Diff #175193)

Agreed.
I don't have commit privs; could someone commit this for me?

[dblaikie]

include/llvm/Support/BranchProbability.h

131–133 ↗

(On Diff #175193)

Could we do something the other way around?

Could op+ be implemented with either a value passed left parameter?

Alternatively - should this warning/fixes be restricted to types with non-trivial copy construction? Because I'm not sure this code would be made any worse by the code being as-is? I would imagine it might produce the same code with or without your change, once optimized?

lib/Target/AArch64/AArch64ISelLowering.cpp

7845 ↗

(On Diff #175193)

Sure! Done in r347609

[• Quuxplusone]

include/llvm/Support/BranchProbability.h

131–133 ↗

(On Diff #175193)

Could op+ be implemented with either a value passed left parameter?

Yes, and sometimes it's done that way, idiomatically. Then there'd be no question of copy-elision (copy-elision would be impossible by definition). But return lhs += rhs; would still disable implicit move and force the return object to be initialized by copy instead of by move.

In this case, because BranchProbability is trivially copyable, the difference between copy and move doesn't matter; and because it's no bigger than 16 bytes and all these methods are inlined, copy elision doesn't matter either, you're right. Once you get up to 20 bytes, copy elision starts looking good even for trivially copyable types.
https://godbolt.org/z/Tfukus

I still think this should be committed for code-cleanliness purposes (e.g. if we get a clang-tidy check to warn about potentially pessimizing return x @= y constructions, then this "false positive" in the codebase would be annoying)... but I suspect you're right that it cannot possibly matter to the physical codegen in this case.

lib/Target/AArch64/AArch64ISelLowering.cpp

7845 ↗

(On Diff #175193)

Thanks!

[dblaikie]

include/llvm/Support/BranchProbability.h

131–133 ↗

(On Diff #175193)

*nod* I'm wondering whether such a theoretical/actual clang-tidy warning could be improved to not warn on this code, due to the trivial copy/move.

I guess if some of the operations weren't inlined, then LLVM wouldn't understand the aliasing relationships well enough to be able to essentially do NRVO itself? So it'd be hard for such a clang-tidy warning to differentiate this entirely benign case from similar-but-different non-benign cases (even when all cases under cconsideration had trivial move/copy ops, etc)

[• Quuxplusone]

Extend my check to include return ++x, which catches a couple more cases. These are iterator types, which means they are presumably "cheap" to copy; but they aren't trivial, unless my checker has a bug. :)

@steveire suggests that I should try turning my checker into a clang-tidy matcher.

[dblaikie]

Sounds alright to me - thanks!

[• Quuxplusone]

@dblaikie merged one of the original diffs on 2018-11-26, but the rest are still extant.
Rebased on master. Could I get someone to land the remainder of this patch for me and then close it?

llvm/trunk/include/llvm/CodeGen/MachineInstrBundle.h

inline MachineBasicBlock::const_instr_iterator getBundleStart(
   return I;
 }
 
 /// Returns an iterator pointing beyond the bundle containing \p I.
 inline MachineBasicBlock::instr_iterator getBundleEnd(
     MachineBasicBlock::instr_iterator I) {
   while (I->isBundledWithSucc())
     ++I;
-  return ++I;
+  ++I;
+  return I;
 }
 
 /// Returns an iterator pointing beyond the bundle containing \p I.
 inline MachineBasicBlock::const_instr_iterator getBundleEnd(
     MachineBasicBlock::const_instr_iterator I) {
   while (I->isBundledWithSucc())
     ++I;
-  return ++I;
+  ++I;
+  return I;
 }
 
 //===----------------------------------------------------------------------===//
 // MachineOperand iterator
 //
 
 /// MachineOperandIteratorBase - Iterator that can visit all operands on a
 /// MachineInstr, or all operands on a bundle of MachineInstrs.  This class is

llvm/trunk/include/llvm/Support/BranchProbability.h

assert(N != UnknownN &&
            "Unknown probability cannot participate in arithmetics.");
     assert(RHS > 0 && "The divider cannot be zero.");
     N /= RHS;
     return *this;
   }
 
   BranchProbability operator+(BranchProbability RHS) const {
     BranchProbability Prob(*this);
-    return Prob += RHS;
+    Prob += RHS;
+    return Prob;
   }
 
   BranchProbability operator-(BranchProbability RHS) const {
     BranchProbability Prob(*this);
-    return Prob -= RHS;
+    Prob -= RHS;
+    return Prob;
   }
 
   BranchProbability operator*(BranchProbability RHS) const {
     BranchProbability Prob(*this);
-    return Prob *= RHS;
+    Prob *= RHS;
+    return Prob;
   }
 
   BranchProbability operator*(uint32_t RHS) const {
     BranchProbability Prob(*this);
-    return Prob *= RHS;
+    Prob *= RHS;
+    return Prob;
   }
 
   BranchProbability operator/(uint32_t RHS) const {
     BranchProbability Prob(*this);
-    return Prob /= RHS;
+    Prob /= RHS;
+    return Prob;
   }
 
   bool operator==(BranchProbability RHS) const { return N == RHS.N; }
   bool operator!=(BranchProbability RHS) const { return !(*this == RHS); }
 
   bool operator<(BranchProbability RHS) const {
     assert(N != UnknownN && RHS.N != UnknownN &&
            "Unknown probability cannot participate in comparisons.");

llvm/trunk/lib/Support/Path.cpp

ptrdiff_t const_iterator::operator-(const const_iterator &RHS) const {
   return Position - RHS.Position;
 }
 
 reverse_iterator rbegin(StringRef Path, Style style) {
   reverse_iterator I;
   I.Path = Path;
   I.Position = Path.size();
   I.S = style;
-  return ++I;
+  ++I;
+  return I;
 }
 
 reverse_iterator rend(StringRef Path) {
   reverse_iterator I;
   I.Path = Path;
   I.Component = Path.substr(0, 0);
   I.Position = 0;
   return I;

llvm/trunk/lib/Target/X86/X86MCInstLower.cpp

 PrevCrossBBInst(MachineBasicBlock::const_iterator MBBI) {
   const MachineBasicBlock *MBB = MBBI->getParent();
   while (MBBI == MBB->begin()) {
     if (MBB == &MBB->getParent()->front())
       return MachineBasicBlock::const_iterator();
     MBB = MBB->getPrevNode();
     MBBI = MBB->end();
   }
-  return --MBBI;
+  --MBBI;
+  return MBBI;
 }
 
 static const Constant *getConstantFromPool(const MachineInstr &MI,
                                            const MachineOperand &Op) {
   if (!Op.isCPI() || Op.getOffset() != 0)
     return nullptr;
 
   ArrayRef<MachineConstantPoolEntry> Constants =

llvm/trunk/lib/Transforms/Instrumentation/Instrumentation.cpp

 #include "llvm/InitializePasses.h"
 #include "llvm/PassRegistry.h"
 
 using namespace llvm;
 
 /// Moves I before IP. Returns new insert point.
 static BasicBlock::iterator moveBeforeInsertPoint(BasicBlock::iterator I, BasicBlock::iterator IP) {
   // If I is IP, move the insert point down.
-  if (I == IP)
-    return ++IP;
+  if (I == IP) {
+    ++IP;
+  } else {
     // Otherwise, move I before IP and return IP.
     I->moveBefore(&*IP);
+  }
   return IP;
 }
 
 /// Instrumentation passes often insert conditional checks into entry blocks.
 /// Call this function before splitting the entry block to move instructions
 /// that must remain in the entry block up before the split point. Static
 /// allocas and llvm.localescape calls, for example, must remain in the entry
 /// block.