Page MenuHomePhabricator

New warning -Wpessimizing-move to catch when removing calls to std::move prevent optimization
ClosedPublic

Authored by rtrieu on Feb 13 2015, 3:55 PM.

Details

Summary

This warning will catch:

  1. Using std::move in a return where the moved object is a local variable
  2. Using std::move on a pr-value object
struct A {}
A test() {
  A a = std::move(A());  // warn since A() is a pr-value
  return std::move(a);  // warn since a is a local variable
}

Diff Detail

Repository
rL LLVM

Event Timeline

rtrieu updated this revision to Diff 19944.Feb 13 2015, 3:55 PM
rtrieu retitled this revision from to New warning -Wpessimizing-move to catch when removing calls to std::move in return statements will result in NRVO.
rtrieu updated this object.
rtrieu edited the test plan for this revision. (Show Details)
rtrieu updated this object.
rtrieu added a subscriber: Unknown Object (MLST).
rsmith added a subscriber: rsmith.Feb 18 2015, 5:29 PM

I don't think it's appropriate for this warning to look at whether we would have actually performed NRVO. Instead, I would suggest modeling this as follows: when we perform initialization, and the source expression is std::move(x), and x is of class type, and either x is a prvalue or {we're initializing a return object and x is the name of a local variable or parameter of the returned type ignoring cv-qualification}, emit a warning.

So then something like this:

struct S{};
S test(bool cond) {
  S s1, s2;
  if (cond)
    return std::move(s1);
  return std::move(s2);
}

would be better with the std::move's removed:

struct S{};
S test(bool cond) {
  S s1, s2;
  if (cond)
    return s1;
  return s2;
}

even though NRVO would not be triggered since different local variables are used?

rtrieu updated this revision to Diff 20251.Feb 18 2015, 6:40 PM

Forgot to add the tests to the patch.

rtrieu updated this revision to Diff 23981.Apr 17 2015, 6:31 PM
rtrieu retitled this revision from New warning -Wpessimizing-move to catch when removing calls to std::move in return statements will result in NRVO to New warning -Wpessimizing-move to catch when removing calls to std::move prevent optimization.
rtrieu updated this object.
rsmith added inline comments.Apr 17 2015, 7:31 PM
lib/Sema/SemaInit.cpp
3245–3246 ↗(On Diff #23981)

This check is not especially cheap, and the code below will be cheap in most cases; it's probably better to omit this.

3249 ↗(On Diff #23981)

Maybe IgnoreParens before the cast?

3259 ↗(On Diff #23981)

Maybe IgnoreParens here too.

3392–3393 ↗(On Diff #23981)

You should defer the diagnostic until we perform the InitializationSequence. We might select a different initialization sequence or overload candidate and not actually call this constructor.

lib/Sema/SemaStmt.cpp
3049 ↗(On Diff #23981)

This looks familiar. Can some commonality be factored out here?

Or, better, can you implement both checks in a single place? When performing the initialization sequence, you can ask the initialized entity whether it's the returned object in a return statement.

3080–3081 ↗(On Diff #23981)

If the types don't match, you aren't likely to have a pessimizing move (elision would only occur if your return type's constructor took its argument by value), but perhaps we should still produce some kind of "redundant move" warning. (Per http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#1579, you get an implicit move no matter what the types are.)

rtrieu updated this revision to Diff 24262.Apr 22 2015, 4:55 PM

Consolidate the checking to SemaInit.cpp
Ass warning for redundant move on return

rtrieu added inline comments.Apr 22 2015, 4:58 PM
lib/Sema/SemaInit.cpp
3245–3246 ↗(On Diff #23981)

isIgnored check removed.

3249 ↗(On Diff #23981)

More parens removed.

3259 ↗(On Diff #23981)

More parens removed. Plus test cases for parens.

3392–3393 ↗(On Diff #23981)

Moved check to after InitializationSequence in PerformCopyInitiailization.

lib/Sema/SemaStmt.cpp
3049 ↗(On Diff #23981)

Moved to a single checking function in SemaInit

3080–3081 ↗(On Diff #23981)

Added new warning for redundant moves on return.

rsmith added inline comments.Apr 22 2015, 8:25 PM
lib/Sema/SemaInit.cpp
7346 ↗(On Diff #24262)

The reference type check here is redundant: a record type is never a reference type.

7352–7353 ↗(On Diff #24262)

This check seems to prevent your redundant_move_on_return warning from firing at the right times.

7377–7378 ↗(On Diff #24262)

You should also check that VD is from the current function/lambda/whatever and not an enclosing one (using DRE->refersToEnclosingVariableOrCapture).

7420–7423 ↗(On Diff #24262)

Can you put this inside Perform so that all the users of InitializationSequence are guaranteed to reach it? There's lots of places that perform copy elision but don't use this convenience wrapper.

test/SemaCXX/warn-pessmizing-move.cpp
38–43 ↗(On Diff #24262)

We should give the "redundant move" warning for this case, but we won't because we didn't use a copy or move constructor.

rtrieu updated this revision to Diff 24587.Apr 28 2015, 5:10 PM

Moved the check into InitiailzationSequence::Perform
Add macro handling for the fix-it hints for common cases
Capture additional cases for the redundant move

rtrieu added inline comments.Apr 28 2015, 5:16 PM
lib/Sema/SemaInit.cpp
7346 ↗(On Diff #24262)

Removed reference check.

7352–7353 ↗(On Diff #24262)

Peeled back more nodes in some cases of redundant move, including MaterializeTemporaryExpr's and additional CXXConstructorExpr's.

7377–7378 ↗(On Diff #24262)

Check added.

7420–7423 ↗(On Diff #24262)

Moved check to InitializationSequence::Perform.

test/SemaCXX/warn-pessmizing-move.cpp
38–43 ↗(On Diff #24262)

Added this test case to the redundant move test.

rsmith accepted this revision.Apr 28 2015, 5:23 PM
rsmith added a reviewer: rsmith.

LGTM with a couple of minor changes.

lib/Sema/SemaInit.cpp
5768–5769 ↗(On Diff #24587)

If CCE is a CXXTemporaryObjectExpr (which derives from CXXConstructExpr), you shouldn't step through it here -- CXXTemporaryObjectExpr is always modeling explicit syntax.

5770 ↗(On Diff #24587)

It would be marginally better to check

CCE->getNumArgs() == 0 ||
(CCE->getNumArgs() > 1 && !isa<CXXDefaultArgExpr>(CCE->getArg(1)))

in case the constructor has a default argument.

This revision is now accepted and ready to land.Apr 28 2015, 5:23 PM
This revision was automatically updated to reflect the committed changes.

Submitted with changes.