Skip to content

Conversation

@aaupov
Copy link
Contributor

@aaupov aaupov commented Jul 18, 2024

AddressProbesMap is keyed by binary addresses, and it makes sense to
treat them as ordered. This also enables slicing by binary function/
binary basic block, to be used in BOLT
(#99554).

Test Plan: NFC

aaupov added 2 commits July 18, 2024 12:34
Created using spr 1.3.4
@aaupov aaupov requested a review from wlei-llvm July 18, 2024 19:57
Copy link
Contributor

@wlei-llvm wlei-llvm left a comment

Choose a reason for hiding this comment

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

LGTM.

aaupov added 2 commits July 18, 2024 20:59
Created using spr 1.3.4

[skip ci]
Created using spr 1.3.4
@aaupov aaupov changed the base branch from users/aaupov/spr/main.mcnfc-use-stdmap-for-addressprobesmap to main July 19, 2024 04:00
@aaupov aaupov merged commit 6c3aa62 into main Jul 19, 2024
@aaupov aaupov deleted the users/aaupov/spr/mcnfc-use-stdmap-for-addressprobesmap branch July 19, 2024 04:00
@WenleiHe
Copy link
Member

AddressProbesMap is keyed by binary addresses, and it makes sense to treat them as ordered.

@aaupov this comment doesn't make sense. unordered_map is faster, and it was intentional. Order wasn't needed.

The intention of this patch is only obvious with the other patch that uses the map, so it'd be better to merge the two in one patch.

@aaupov
Copy link
Contributor Author

aaupov commented Jul 19, 2024

AddressProbesMap is keyed by binary addresses, and it makes sense to treat them as ordered.

@aaupov this comment doesn't make sense. unordered_map is faster, and it was intentional. Order wasn't needed.

Sorry, the wording wasn't ideal. I was trying to convey that addresses can be treated as ordered keys (unlike e.g. GUIDs), which allows certain lookups as in the follow-up diff.

The intention of this patch is only obvious with the other patch that uses the map, so it'd be better to merge the two in one patch.

I see your point, but I tried to explain the intention in the message. I applied the usual logic of keeping changes small and isolated, but perhaps in this case the changes were best left coupled.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants