Remove std::move for trivially copyable types #23056

apivovarov · 2025-02-25T02:27:38Z

Changes:

Removed unnecessary std::move calls for some trivially_copyable classes
Literal::CopyFrom expects a reference (&), not an r-value reference (&&). Removed std::move on the first parameter.

apivovarov · 2025-03-10T19:30:13Z

xla/hlo/evaluator/hlo_evaluator.cc

@@ -318,11 +324,11 @@ std::optional<ParamIndexAndValue> TryParsingInstructionAsParameterAndInteger(
  }
  std::optional<DynamicOrStaticInteger> integer_value =
      GetInstructionValueAsInteger(instruction, precomputed_analyses);
-  result.value = std::move(integer_value);
+  result.value = integer_value;


removing std::move because DynamicOrStaticInteger is trivially copyable, its copy and move operations behave identically because it has no special resources

apivovarov · 2025-03-10T19:31:37Z

xla/hlo/evaluator/hlo_evaluator.cc

  if (!result.IsValid()) {
    return std::nullopt;
  }
-  return std::optional<ParamIndexAndValue>(std::move(result));
+  return result;


removing std::move because ParamIndexAndValue is trivially copyable.

removing std::optional too because wrapping result with std::optional explicitly is not necessary - the result will be implicitly converted to std::optional

apivovarov · 2025-03-10T19:32:50Z

xla/hlo/evaluator/hlo_evaluator.cc

@@ -377,8 +383,7 @@ std::optional<WhileCondComparisonOrNoOp> PatternMatchLoopCondComparison(
  if (!lhs.has_value() || !rhs.has_value()) {
    return std::nullopt;
  }
-  return WhileCondComparison{comparison->comparison_direction(),
-                             *std::move(lhs), *std::move(rhs)};
+  return WhileCondComparison{comparison->comparison_direction(), *lhs, *rhs};


lhs and rhs are ParamIndexAndValue which is trivially copyable - removing std::move

apivovarov · 2025-03-10T19:34:07Z

xla/hlo/evaluator/hlo_evaluator.cc

@@ -1696,7 +1701,7 @@ absl::Status HloEvaluator::HandleTuple(const HloInstruction* tuple) {
    CHECK(new_result.IsDetermined(visitor_shape_index_));
    Literal literal;
    TF_RETURN_IF_ERROR(
-        literal.CopyFrom(std::move(new_result),
+        literal.CopyFrom(new_result,


Literal::CopyFrom expects a reference (&), not an r-value reference (&&). Removing std::move

apivovarov · 2025-03-10T19:34:40Z

xla/stream_executor/kernel.h

-  void set_metadata(KernelMetadata metadata) {
-    metadata_ = std::move(metadata);
-  }
+  void set_metadata(KernelMetadata metadata) { metadata_ = metadata; }


removing std::move because KernelMetadata is trivially copyable.

apivovarov · 2025-03-10T19:38:50Z

Hi Eugene
I've just rebased this PR and added inline comments to justify each change.

Could you take a look when you have a moment? @ezhulenev

apivovarov · 2025-03-20T21:25:27Z

Hi Reed, It looks like this PR may have gotten lost among the others. Could you take a look and let me know your thoughts? @reedwm

reedwm · 2025-03-20T23:11:26Z

xla/hlo/evaluator/hlo_evaluator.cc

+static_assert(std::is_trivially_copyable_v<DynamicOrStaticInteger>,
+              "DynamicOrStaticInteger is expected to be trivially copyable");


I personally don't think it's worth static_asserting these types are trivially copyable. Without the context of this PR, it's non-obvious to someone reading the code why there is a static assert, and in the very-unlikely case someone does add a non-trivially-copyable field to these types, the only downside will be negligible lower performance since we don't std::move it.

Makes sense, removed the static_asserts.

Imported from GitHub PR #23056 Changes: - Removed unnecessary std::move calls for some trivially_copyable classes - Literal::CopyFrom expects a reference (&), not an r-value reference (&&). Removed std::move on the first parameter. Copybara import of the project: -- 6051f15 by Alexander Pivovarov <[email protected]>: Remove std::move for trivially copyable types Merging this change closes #23056 FUTURE_COPYBARA_INTEGRATE_REVIEW=#23056 from apivovarov:no_move_for_KernelMetadata 6051f15 PiperOrigin-RevId: 739064828

Imported from GitHub PR openxla/xla#23056 Changes: - Removed unnecessary std::move calls for some trivially_copyable classes - Literal::CopyFrom expects a reference (&), not an r-value reference (&&). Removed std::move on the first parameter. Copybara import of the project: -- 6051f15383d63210c313ab712eaa3a00c7b0105f by Alexander Pivovarov <[email protected]>: Remove std::move for trivially copyable types Merging this change closes #23056 FUTURE_COPYBARA_INTEGRATE_REVIEW=openxla/xla#23056 from apivovarov:no_move_for_KernelMetadata 6051f15383d63210c313ab712eaa3a00c7b0105f PiperOrigin-RevId: 739064828

Reverts 4ee2886 FUTURE_COPYBARA_INTEGRATE_REVIEW=#23056 from apivovarov:no_move_for_KernelMetadata 6051f15 PiperOrigin-RevId: 731692216

FUTURE_COPYBARA_INTEGRATE_REVIEW=openxla/xla#23056 from apivovarov:no_move_for_KernelMetadata 6051f15383d63210c313ab712eaa3a00c7b0105f PiperOrigin-RevId: 731692216

Imported from GitHub PR openxla/xla#23056 Changes: - Removed unnecessary std::move calls for some trivially_copyable classes - Literal::CopyFrom expects a reference (&), not an r-value reference (&&). Removed std::move on the first parameter. Copybara import of the project: -- 6051f15383d63210c313ab712eaa3a00c7b0105f by Alexander Pivovarov <[email protected]>: Remove std::move for trivially copyable types Merging this change closes #23056 PiperOrigin-RevId: 739114605

…mits/cdb53266e6c251d91a2c321d64e8466caff129a9). Reverts 4ee2886 FUTURE_COPYBARA_INTEGRATE_REVIEW=#23056 from apivovarov:no_move_for_KernelMetadata 6051f15 PiperOrigin-RevId: 737986828

This is to match the recently added API in pthreadpool.h from google/pthreadpool@bd09d5c Otherwise, we would get a linker error when building xnn_threadpool_test: ``` ld: error: undefined symbol: pthreadpool_parallelize_4d_tile_2d_dynamic ``` Command to test: ``` bazel build -c opt --define=pthreadpool_header_only=true \ //xla/backends/cpu/runtime/xnnpack:xnn_threadpool_test ``` Reverts 4ee2886 FUTURE_COPYBARA_INTEGRATE_REVIEW=#23056 from apivovarov:no_move_for_KernelMetadata 6051f15 PiperOrigin-RevId: 739070530

…mits/cdb53266e6c251d91a2c321d64e8466caff129a9). Reverts 4ee2886 FUTURE_COPYBARA_INTEGRATE_REVIEW=#23056 from apivovarov:no_move_for_KernelMetadata 6051f15 PiperOrigin-RevId: 737986828

…tform API equivalent. Reverts changelist 739108726 FUTURE_COPYBARA_INTEGRATE_REVIEW=#23056 from apivovarov:no_move_for_KernelMetadata 6051f15 PiperOrigin-RevId: 739132749

Reverts 4ee2886 FUTURE_COPYBARA_INTEGRATE_REVIEW=#23056 from apivovarov:no_move_for_KernelMetadata 6051f15 PiperOrigin-RevId: 739119656

…ectiveOpGroupMode. It is not very obvious from the code or comments, but we can use `GetReplicaGroupCountAndSize` and `GetParticipatingFlattenedIdGroups` only for `AllReduce` and `AllGather`, because of the check in `GetCollectiveUseGlobalDeviceIds`. There is no reason why `CollectiveOpGroupMode` of other instruction would not be enough to get the information. The check of `AllReduce` and `AllGather` was added in #20024, but I couldn't find explanation why it wouldn't work of other instruction. Reverts 4ee2886 FUTURE_COPYBARA_INTEGRATE_REVIEW=#23056 from apivovarov:no_move_for_KernelMetadata 6051f15 PiperOrigin-RevId: 739163964

…mits/cdb53266e6c251d91a2c321d64e8466caff129a9). Reverts 4ee2886 FUTURE_COPYBARA_INTEGRATE_REVIEW=#23056 from apivovarov:no_move_for_KernelMetadata 6051f15 PiperOrigin-RevId: 737986828

…ectiveOpGroupMode. It is not very obvious from the code or comments, but we can use `GetReplicaGroupCountAndSize` and `GetParticipatingFlattenedIdGroups` only for `AllReduce` and `AllGather`, because of the check in `GetCollectiveUseGlobalDeviceIds`. There is no reason why `CollectiveOpGroupMode` of other instruction would not be enough to get the information. The check of `AllReduce` and `AllGather` was added in #20024, but I couldn't find explanation why it wouldn't work of other instruction. Reverts 4ee2886 FUTURE_COPYBARA_INTEGRATE_REVIEW=#23056 from apivovarov:no_move_for_KernelMetadata 6051f15 PiperOrigin-RevId: 739163964

FUTURE_COPYBARA_INTEGRATE_REVIEW=#23056 from apivovarov:no_move_for_KernelMetadata 6051f15 PiperOrigin-RevId: 739257603

apivovarov requested a review from ezhulenev February 25, 2025 02:28

apivovarov force-pushed the no_move_for_KernelMetadata branch from 3a4e2c9 to 804e172 Compare February 25, 2025 02:29

apivovarov force-pushed the no_move_for_KernelMetadata branch from 804e172 to 0d19b1e Compare March 10, 2025 19:22

apivovarov commented Mar 10, 2025

View reviewed changes

apivovarov force-pushed the no_move_for_KernelMetadata branch from 0d19b1e to 8aeb460 Compare March 11, 2025 18:50

apivovarov force-pushed the no_move_for_KernelMetadata branch from 8aeb460 to 37b8963 Compare March 20, 2025 21:21

reedwm requested changes Mar 20, 2025

View reviewed changes

Remove std::move for trivially copyable types

6051f15

apivovarov force-pushed the no_move_for_KernelMetadata branch from 37b8963 to 6051f15 Compare March 21, 2025 03:03

reedwm approved these changes Mar 21, 2025

View reviewed changes

copybara-service bot mentioned this pull request Mar 21, 2025

PR #23056: Remove std::move for trivially copyable types #24022

Merged

copybara-service bot mentioned this pull request Mar 21, 2025

PR #23056: Remove std::move for trivially copyable types tensorflow/tensorflow#89726

Merged

copybara-service bot pushed a commit that referenced this pull request Mar 21, 2025

[XLA:CPU][tfcompile] Use thunk runtime for tfcompiled models.

309c975

Reverts 4ee2886 FUTURE_COPYBARA_INTEGRATE_REVIEW=#23056 from apivovarov:no_move_for_KernelMetadata 6051f15 PiperOrigin-RevId: 731692216

copybara-service bot mentioned this pull request Mar 21, 2025

[XLA:CPU][tfcompile] Use thunk runtime for tfcompiled models. #23352

Open

copybara-service bot mentioned this pull request Mar 21, 2025

[XLA:CPU][tfcompile] Use thunk runtime for tfcompiled models. tensorflow/tensorflow#88552

Draft

copybara-service bot mentioned this pull request Mar 21, 2025

Integrate Triton up to [cdb5326](https://github.com/openai/triton/commits/cdb53266e6c251d91a2c321d64e8466caff129a9). #24025

Open

copybara-service bot mentioned this pull request Mar 21, 2025

[xla:cpu] Add new pthreadpool API to XNN thread pool integration #24020

Merged

copybara-service bot mentioned this pull request Mar 21, 2025

Replace outdated select() on --cpu in compiler/xla/tsl/BUILD with platform API equivalent. #24031

Merged

copybara-service bot pushed a commit that referenced this pull request Mar 21, 2025

[XLA:GPU] Add a microbenchmark for scatter from b/404266433.

6d0f148

Reverts 4ee2886 FUTURE_COPYBARA_INTEGRATE_REVIEW=#23056 from apivovarov:no_move_for_KernelMetadata 6051f15 PiperOrigin-RevId: 739119656

copybara-service bot mentioned this pull request Mar 21, 2025

[XLA:GPU] Add a microbenchmark for scatter from b/404266433. #24032

Open

copybara-service bot mentioned this pull request Mar 21, 2025

[XLA:Collective] Get participant ids, count and group sizes from CollectiveOpGroupMode. #24035

Merged

copybara-service bot pushed a commit that referenced this pull request Mar 21, 2025

[XLA:CPU] Fix use-after-move in CpuCompiler::CompileAheadOfTime

50fa670

FUTURE_COPYBARA_INTEGRATE_REVIEW=#23056 from apivovarov:no_move_for_KernelMetadata 6051f15 PiperOrigin-RevId: 739257603

copybara-service bot mentioned this pull request Mar 21, 2025

[XLA:CPU] Fix use-after-move in CpuCompiler::CompileAheadOfTime #24051

Merged

copybara-service bot closed this in 8e58e1c Mar 21, 2025

copybara-service bot pushed a commit that referenced this pull request Mar 21, 2025

[XLA:CPU] Fix use-after-move in CpuCompiler::CompileAheadOfTime

1580f6e

FUTURE_COPYBARA_INTEGRATE_REVIEW=#23056 from apivovarov:no_move_for_KernelMetadata 6051f15 PiperOrigin-RevId: 739257603

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove std::move for trivially copyable types #23056

Remove std::move for trivially copyable types #23056

apivovarov commented Feb 25, 2025 •

edited

Loading

apivovarov Mar 10, 2025

apivovarov Mar 10, 2025 •

edited

Loading

apivovarov Mar 10, 2025

apivovarov Mar 10, 2025

apivovarov Mar 10, 2025

apivovarov commented Mar 10, 2025

apivovarov commented Mar 20, 2025

reedwm Mar 20, 2025

apivovarov Mar 21, 2025

		static_assert(std::is_trivially_copyable_v<DynamicOrStaticInteger>,
		"DynamicOrStaticInteger is expected to be trivially copyable");

Remove std::move for trivially copyable types #23056

Remove std::move for trivially copyable types #23056

Conversation

apivovarov commented Feb 25, 2025 • edited Loading

apivovarov Mar 10, 2025

Choose a reason for hiding this comment

apivovarov Mar 10, 2025 • edited Loading

Choose a reason for hiding this comment

apivovarov Mar 10, 2025

Choose a reason for hiding this comment

apivovarov Mar 10, 2025

Choose a reason for hiding this comment

apivovarov Mar 10, 2025

Choose a reason for hiding this comment

apivovarov commented Mar 10, 2025

apivovarov commented Mar 20, 2025

reedwm Mar 20, 2025

Choose a reason for hiding this comment

apivovarov Mar 21, 2025

Choose a reason for hiding this comment

apivovarov commented Feb 25, 2025 •

edited

Loading

apivovarov Mar 10, 2025 •

edited

Loading