This is an archive of the discontinued LLVM Phabricator instance.

[Orc] Fix copy elision warning in RPCUtils
ClosedPublic

Authored by sgraenitz on Mar 19 2021, 7:05 AM.

Details

Summary

The callB() template function always moved errors on return, because in the majority of cases its return type is an Expected<T> and the error must be moved into the implicit ctor.
For the special case of a void result, however, the ResultTraits class is specialized and the return type is a raw Error. Some build bots complain, that in favor of NRVO errors should not be moved in this case.

llvm/include/llvm/ExecutionEngine/Orc/Shared/RPCUtils.h:1513:27:
llvm/include/llvm/ExecutionEngine/Orc/Shared/RPCUtils.h:1519:27:
llvm/include/llvm/ExecutionEngine/Orc/Shared/RPCUtils.h:1526:29:
  warning: moving a local object in a return statement prevents copy elision [-Wpessimizing-move]

The warning is reasonable from a type-system point of view. For performance it's entirely insignificant.

Diff Detail

Event Timeline

sgraenitz requested review of this revision.Mar 19 2021, 7:05 AM
sgraenitz created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMar 19 2021, 7:05 AM

I actually didn't manage to reproduce the warning with GCC 10 on Debian, so I couldn't really test this. @thakis Do you want to run a pre-test? Otherwise, we could land it right away and see if it is effective.

@lhames Correct me if I am wrong, but I think consumeAbandoned() should have been called on detail::ResultTraits<AltRetT> and not on detail::ResultTraits<typename Func::ReturnType> in the first place. I simplified it now with RTraits analog to appendCallNB(). Hope it's fine.

Landing and checking is easier. I saw the warnings on some bot; it's easier to check bot logs after this landed :)

sgraenitz updated this revision to Diff 331881.Mar 19 2021, 8:01 AM

Make returnError() static (oops). Rename RTraits -> AltRetTraits. Turn some autos into concrete typenames to aid readability.

lhames accepted this revision.Mar 22 2021, 9:26 AM

LGTM. Thanks very much for fixing this Stefan!

This revision is now accepted and ready to land.Mar 22 2021, 9:26 AM
This revision was automatically updated to reflect the committed changes.