[MC][profgen][NFC] Expand auto for MCDecodedPseudoProbe

[aaupov]

Expand autos in select places in preparation to #102789.

[llvmbot]

@llvm/pr-subscribers-mc

@llvm/pr-subscribers-pgo

Author: Amir Ayupov (aaupov)

Changes

Expand autos in select places to trigger type casts.

---

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

2 Files Affected:

• (modified) llvm/lib/MC/MCPseudoProbe.cpp (+2-2)
• (modified) llvm/tools/llvm-profgen/ProfileGenerator.cpp (+1-1)

diff --git a/llvm/lib/MC/MCPseudoProbe.cpp b/llvm/lib/MC/MCPseudoProbe.cpp
index bb9538d55b7aa..e9e391c8eadd7 100644
--- a/llvm/lib/MC/MCPseudoProbe.cpp
+++ b/llvm/lib/MC/MCPseudoProbe.cpp
@@ -630,7 +630,7 @@ void MCPseudoProbeDecoder::printProbeForAddress(raw_ostream &OS,
                                                 uint64_t Address) {
   auto It = Address2ProbesMap.find(Address);
   if (It != Address2ProbesMap.end()) {
-    for (auto &Probe : It->second) {
+    for (const MCDecodedPseudoProbe &Probe : It->second) {
       OS << " [Probe]:\t";
       Probe.print(OS, GUID2FuncDescMap, true);
     }
@@ -657,7 +657,7 @@ MCPseudoProbeDecoder::getCallProbeForAddr(uint64_t Address) const {
   const auto &Probes = It->second;
 
   const MCDecodedPseudoProbe *CallProbe = nullptr;
-  for (const auto &Probe : Probes) {
+  for (const MCDecodedPseudoProbe &Probe : Probes) {
     if (Probe.isCall()) {
       // Disabling the assert and returning first call probe seen so far.
       // Subsequent call probes, if any, are ignored. Due to the the way
diff --git a/llvm/tools/llvm-profgen/ProfileGenerator.cpp b/llvm/tools/llvm-profgen/ProfileGenerator.cpp
index 175556c2220e6..c840e7789edfe 100644
--- a/llvm/tools/llvm-profgen/ProfileGenerator.cpp
+++ b/llvm/tools/llvm-profgen/ProfileGenerator.cpp
@@ -1194,7 +1194,7 @@ void ProfileGeneratorBase::extractProbesFromRange(
           Binary->getAddress2ProbesMap();
       auto It = Address2ProbesMap.find(IP.Address);
       if (It != Address2ProbesMap.end()) {
-        for (const auto &Probe : It->second) {
+        for (const MCDecodedPseudoProbe &Probe : It->second) {
           ProbeCounter[&Probe] += Count;
         }
       }

[WenleiHe]

Expand autos in select places to trigger type casts.

Can you explain? auto is a syntax sugar. Probes is of type std::list<MCDecodedPseudoProbe>, so the deduced type for auto is MCDecodedPseudoProbe. What type extra casting do you expect by changing from auto to explicit type?

[aaupov]

In #102789, Probes (when accessed via Address2ProbesMap) become std::vector<std::reference_wrapper<MCDecodedPseudoProbe>>.
auto would expand to a reference_wrapper, so to access the underlying pseudo probe, it would need explicit get. To avoid that, we can use reference_wrapper::operator T& which is functionally identical to get. This is achieved by providing MCDecodedPseudoProbe & as type of a range-based variable.

https://en.cppreference.com/w/cpp/utility/functional/reference_wrapper/get

[WenleiHe]

Ok, then this change should be part of #102789, and should not be separated. Otherwise the motivation and effect of this change is lost, because the base side of this change still use std::list, and the change description stating "trigger type casts" can be confusing.

Suggest you merge it back (or at least fix change description).

[aaupov]

Ok, then this change should be part of #102789, and should not be separated. Otherwise the motivation and effect of this change is lost, because the base side of this change still use std::list, and the change description stating "trigger type casts" can be confusing.

Suggest you merge it back (or at least fix change description).

I intended to minimize the changes in #102789 and make all preparation changes small, isolated, and verifiably NFC. I've changed the description, hope it makes sense now.

[WenleiHe]

Looks good, thanks.

I intended to minimize the changes in #102789 and make all preparation changes small, isolated, and verifiably NFC.

You might have gone a bit too far. :) And this isn't the first instance.

[WenleiHe]

Not a big deal, but still I though it's worth clarifying. Generally the purpose of splitting a patch isn't to strip things into individual patches as much as possible, but to help reviewers: 1) to make sure a patch is manageable in size for review; 2) group changes with logical connection/separation (components/steps). The other change isn't that big anyways, and evidently splitting here actually make it more difficult for reviewers instead of helping them.

[aaupov]

Thank you for clarifying. I'm used to operate under the assumption of extremely limited reviewer time, hence splitting into minimal/standalone changes. If you're fine with slightly larger and more loosely connected changes, that's also good with me.

[WenleiHe]

Your assumption is right, and you do what's needed to help reviewer. More often than not splitting helps, but not always. It's not about personal preference, but what helps reviewer.