Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Aug 11, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
732c302
[𝘀𝗽𝗿] changes to main this commit is based on
aaupov Aug 11, 2024
e20dd59
[𝘀𝗽𝗿] initial version
aaupov Aug 11, 2024
6e733cb
[𝘀𝗽𝗿] changes introduced through rebase
aaupov Aug 11, 2024
7e92ed8
rebase
aaupov Aug 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions llvm/lib/MC/MCPseudoProbe.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -559,7 +559,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);
}
Expand All @@ -586,7 +586,7 @@ MCPseudoProbeDecoder::getCallProbeForAddr(uint64_t Address) const {
const auto &Probes = It->second;

const MCDecodedPseudoProbe *CallProbe = nullptr;
for (const auto &Probe : Probes) {
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@aaupov aaupov Aug 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

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
Expand Down
2 changes: 1 addition & 1 deletion llvm/tools/llvm-profgen/ProfileGenerator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1185,7 +1185,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;
}
}
Expand Down
Loading