Skip to content

Conversation

@kazutakahirata
Copy link
Contributor

No description provided.

@llvmbot
Copy link
Member

llvmbot commented Feb 28, 2025

@llvm/pr-subscribers-tools-llvm-mca

Author: Kazu Hirata (kazutakahirata)

Changes

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

1 Files Affected:

  • (modified) llvm/lib/MCA/InstrBuilder.cpp (+2-4)
diff --git a/llvm/lib/MCA/InstrBuilder.cpp b/llvm/lib/MCA/InstrBuilder.cpp
index 2cb1908695308..2bac99b6309af 100644
--- a/llvm/lib/MCA/InstrBuilder.cpp
+++ b/llvm/lib/MCA/InstrBuilder.cpp
@@ -634,16 +634,14 @@ InstrBuilder::createInstrDescImpl(const MCInst &MCI,
   bool IsVariadic = MCDesc.isVariadic();
   if ((ID->IsRecyclable = !IsVariadic && !IsVariant)) {
     auto DKey = std::make_pair(MCI.getOpcode(), SchedClassID);
-    Descriptors[DKey] = std::move(ID);
-    return *Descriptors[DKey];
+    return *(Descriptors[DKey] = std::move(ID));
   }
 
   auto VDKey = std::make_pair(hashMCInst(MCI), SchedClassID);
   assert(
       !VariantDescriptors.contains(VDKey) &&
       "Expected VariantDescriptors to not already have a value for this key.");
-  VariantDescriptors[VDKey] = std::move(ID);
-  return *VariantDescriptors[VDKey];
+  return *(VariantDescriptors[VDKey] = std::move(ID));
 }
 
 Expected<const InstrDesc &>

@kazutakahirata kazutakahirata merged commit b2525dc into llvm:main Feb 28, 2025
13 checks passed
@kazutakahirata kazutakahirata deleted the cleanup_001_repeated_hash_lookups_llvm_MCA branch February 28, 2025 16:04
auto DKey = std::make_pair(MCI.getOpcode(), SchedClassID);
Descriptors[DKey] = std::move(ID);
return *Descriptors[DKey];
return *(Descriptors[DKey] = std::move(ID));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Apologies if I am a bit late on this review.

Have you considered using insert_or_assign() ?
https://llvm.org/doxygen/DenseMap_8h_source.html#l00306

Copy link
Collaborator

Choose a reason for hiding this comment

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

Using insert_or_assign() should solve the issue in a nicer way. That is, assuming that my understanding of how insert_or_assign() works is correct :) .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could use insert_or_assign, but that would be a bit mouthful though:

    return *(Descriptors[DKey] = std::move(ID));
    return *(Descriptors.insert_or_assign(DKey, std::move(ID)).first->second);

Copy link
Collaborator

Choose a reason for hiding this comment

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

Mmm yeah, it forces you to do that annoying first->second to get the iterator from the pair.
Nevermind then. Thanks for the fix!

cheezeburglar pushed a commit to cheezeburglar/llvm-project that referenced this pull request Feb 28, 2025
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.

4 participants