Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TableGen] Use std::move instead of swap. NFC. #81606

Merged
merged 1 commit into from
Feb 13, 2024

Conversation

jayfoad
Copy link
Contributor

@jayfoad jayfoad commented Feb 13, 2024

Historically TableGen has used A.swap(B) to move containers without
the expense of copying them. Perhaps this predated rvalue references. In
any case A = std::move(B) seems like a more direct way to implement
this when only A is required after the operation.

Historically TableGen has used `A.swap(B)` to move containers without
the expense of copying them. Perhaps this predated rvalue references. In
any case `A = std::move(B)` seems like a more direct way to implement
this when only A is required after the operation.
@llvmbot
Copy link
Collaborator

llvmbot commented Feb 13, 2024

@llvm/pr-subscribers-llvm-globalisel

Author: Jay Foad (jayfoad)

Changes

Historically TableGen has used A.swap(B) to move containers without
the expense of copying them. Perhaps this predated rvalue references. In
any case A = std::move(B) seems like a more direct way to implement
this when only A is required after the operation.


Full diff: https://github.com/llvm/llvm-project/pull/81606.diff

5 Files Affected:

  • (modified) llvm/utils/TableGen/AsmMatcherEmitter.cpp (+4-5)
  • (modified) llvm/utils/TableGen/CodeGenRegisters.cpp (+3-3)
  • (modified) llvm/utils/TableGen/CodeGenSchedule.cpp (+1-1)
  • (modified) llvm/utils/TableGen/GlobalISelMatchTable.cpp (+2-2)
  • (modified) llvm/utils/TableGen/SubtargetEmitter.cpp (+1-1)
diff --git a/llvm/utils/TableGen/AsmMatcherEmitter.cpp b/llvm/utils/TableGen/AsmMatcherEmitter.cpp
index 9065885618069b..209df25c1fccb4 100644
--- a/llvm/utils/TableGen/AsmMatcherEmitter.cpp
+++ b/llvm/utils/TableGen/AsmMatcherEmitter.cpp
@@ -1270,11 +1270,10 @@ void AsmMatcherInfo::buildRegisterClasses(
       }
 
       RegisterSet Tmp;
-      std::swap(Tmp, ContainingSet);
-      std::insert_iterator<RegisterSet> II(ContainingSet,
-                                           ContainingSet.begin());
-      std::set_intersection(Tmp.begin(), Tmp.end(), RS.begin(), RS.end(), II,
-                            LessRecordByID());
+      std::insert_iterator<RegisterSet> II(Tmp, Tmp.begin());
+      std::set_intersection(ContainingSet.begin(), ContainingSet.end(),
+                            RS.begin(), RS.end(), II, LessRecordByID());
+      ContainingSet = std::move(Tmp);
     }
 
     if (!ContainingSet.empty()) {
diff --git a/llvm/utils/TableGen/CodeGenRegisters.cpp b/llvm/utils/TableGen/CodeGenRegisters.cpp
index 25f38648366300..0f0044957c9c6a 100644
--- a/llvm/utils/TableGen/CodeGenRegisters.cpp
+++ b/llvm/utils/TableGen/CodeGenRegisters.cpp
@@ -1964,9 +1964,9 @@ void CodeGenRegBank::pruneUnitSets() {
   for (unsigned i = 0, e = SuperSetIDs.size(); i != e; ++i) {
     unsigned SuperIdx = SuperSetIDs[i];
     PrunedUnitSets[i].Name = RegUnitSets[SuperIdx].Name;
-    PrunedUnitSets[i].Units.swap(RegUnitSets[SuperIdx].Units);
+    PrunedUnitSets[i].Units = std::move(RegUnitSets[SuperIdx].Units);
   }
-  RegUnitSets.swap(PrunedUnitSets);
+  RegUnitSets = std::move(PrunedUnitSets);
 }
 
 // Create a RegUnitSet for each RegClass that contains all units in the class
@@ -2140,7 +2140,7 @@ void CodeGenRegBank::computeRegUnitSets() {
     if (RCUnitSetsIdx == RegClassUnitSets.size()) {
       // Create a new list of UnitSets as a "fake" register class.
       RegClassUnitSets.resize(RCUnitSetsIdx + 1);
-      RegClassUnitSets[RCUnitSetsIdx].swap(RUSets);
+      RegClassUnitSets[RCUnitSetsIdx] = std::move(RUSets);
     }
   }
 }
diff --git a/llvm/utils/TableGen/CodeGenSchedule.cpp b/llvm/utils/TableGen/CodeGenSchedule.cpp
index 9cebc427dbdbc7..e56bf5bdee634b 100644
--- a/llvm/utils/TableGen/CodeGenSchedule.cpp
+++ b/llvm/utils/TableGen/CodeGenSchedule.cpp
@@ -1788,7 +1788,7 @@ void CodeGenSchedModels::inferFromRW(ArrayRef<unsigned> OperWrites,
     for (const PredTransition &Trans : LastTransitions)
       SubstitutedAny |= Transitions.substituteVariants(Trans);
     LLVM_DEBUG(Transitions.dump());
-    LastTransitions.swap(Transitions.TransVec);
+    LastTransitions = std::move(Transitions.TransVec);
   } while (SubstitutedAny);
 
   // WARNING: We are about to mutate the SchedClasses vector. Do not refer to
diff --git a/llvm/utils/TableGen/GlobalISelMatchTable.cpp b/llvm/utils/TableGen/GlobalISelMatchTable.cpp
index f7166ead9adc3d..d1bdc30849a7f6 100644
--- a/llvm/utils/TableGen/GlobalISelMatchTable.cpp
+++ b/llvm/utils/TableGen/GlobalISelMatchTable.cpp
@@ -545,8 +545,8 @@ void GroupMatcher::optimize() {
     if (T != E)
       F = ++T;
   }
-  optimizeRules<GroupMatcher>(Matchers, MatcherStorage).swap(Matchers);
-  optimizeRules<SwitchMatcher>(Matchers, MatcherStorage).swap(Matchers);
+  Matchers = optimizeRules<GroupMatcher>(Matchers, MatcherStorage);
+  Matchers = optimizeRules<SwitchMatcher>(Matchers, MatcherStorage);
 }
 
 //===- SwitchMatcher ------------------------------------------------------===//
diff --git a/llvm/utils/TableGen/SubtargetEmitter.cpp b/llvm/utils/TableGen/SubtargetEmitter.cpp
index b1502eaa20712a..ebe39167703c8c 100644
--- a/llvm/utils/TableGen/SubtargetEmitter.cpp
+++ b/llvm/utils/TableGen/SubtargetEmitter.cpp
@@ -1649,7 +1649,7 @@ static void collectProcessorIndices(const CodeGenSchedClass &SC,
     IdxVec PI;
     std::set_union(&T.ProcIndex, &T.ProcIndex + 1, ProcIndices.begin(),
                    ProcIndices.end(), std::back_inserter(PI));
-    ProcIndices.swap(PI);
+    ProcIndices = std::move(PI);
   }
 }
 

@jayfoad
Copy link
Contributor Author

jayfoad commented Feb 13, 2024

The motivation is just to write the code in a more obvious way. As a secondary effect this could conceivably reduce peak memory usage, since the memory for B could be freed slightly earlier; but I am not expecting that to be measurable.

Comment on lines +548 to +549
Matchers = optimizeRules<GroupMatcher>(Matchers, MatcherStorage);
Matchers = optimizeRules<SwitchMatcher>(Matchers, MatcherStorage);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No std::moves here since we would be moving from a temporary, and the compiler warns that this prevents copy elision.

Copy link
Contributor

@s-barannikov s-barannikov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW There is also llvm::set_intersect that could modify ContainingSet in-place. It does not accept a '<' comparator though, so migh not be suitable.

@jayfoad jayfoad merged commit f7cddf8 into llvm:main Feb 13, 2024
5 of 6 checks passed
@dwblaikie
Copy link
Collaborator

Historically TableGen has used A.swap(B) to move containers without the expense of copying them. Perhaps this predated rvalue references. In any case A = std::move(B) seems like a more direct way to implement this when only A is required after the operation.

That's certainly a throwback indeed - nice cleanup!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants