This is an archive of the discontinued LLVM Phabricator instance.

[NFC][TLI] Replace std::lower_bound call in getLibFunc with DenseMap lookup
ClosedPublic

Authored by 0xdc03 on Aug 14 2023, 11:42 PM.

Details

Summary

While std::lower_bound takes logarithmic time (relative to the length of
the array) to execute, DenseMap gives better performance characteristics
as it traverses few (if any) elements when collisions do occur,
especially when the number of elements are known in advance.

This gives a speedup of 0.24%:
https://llvm-compile-time-tracker.com/compare.php?from=ac00cca3d9c6c3e9118ebbe47aa5b3ba1ee7404f&to=7f3d4c8ce8cee3a236a2328e46b2a8374672b46e&stat=instructions:u

Diff Detail

Event Timeline

0xdc03 created this revision.Aug 14 2023, 11:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 14 2023, 11:42 PM
0xdc03 requested review of this revision.Aug 14 2023, 11:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 14 2023, 11:42 PM
nikic added inline comments.Aug 15 2023, 2:04 AM
llvm/lib/Analysis/TargetLibraryInfo.cpp
967

This initialization isn't thread-safe. I believe the proper pattern is static const DenseMap<StringRef, unsigned> Indices = buildIndexMap(); or similar.

0xdc03 updated this revision to Diff 550248.Aug 15 2023, 4:09 AM
  • Address reviewer comment
    • Fix thread safety issue
0xdc03 edited the summary of this revision. (Show Details)Aug 15 2023, 4:10 AM
nikic added inline comments.Aug 15 2023, 5:20 AM
llvm/lib/Analysis/TargetLibraryInfo.cpp
945

StandardNames is a global variable in this file, no need to pass it as an argument.

946

Might be a bit more elegant to make this a map to LibFunc rather than unsigned?

0xdc03 updated this revision to Diff 550281.Aug 15 2023, 5:26 AM
  • Change map from unsigned to LibFunc
0xdc03 marked 2 inline comments as done.Aug 15 2023, 5:26 AM
0xdc03 added inline comments.
llvm/lib/Analysis/TargetLibraryInfo.cpp
945

Unfortunately not:

error: 'StandardNames' is a private member of 'llvm::TargetLibraryInfoImpl'
  for (const auto &Func : TargetLibraryInfoImpl::StandardNames)
                                                 ^

I could make buildIndexMap a member of TargetLibraryInfoImpl

nikic added inline comments.Aug 15 2023, 5:29 AM
llvm/lib/Analysis/TargetLibraryInfo.cpp
945

Oh, I see. In that case I'd suggest to make the argument ArrayRef<StringLiteral> StandardNames to avoid any weird array passing semantics.

0xdc03 updated this revision to Diff 550283.Aug 15 2023, 5:33 AM
  • Use a better array type
0xdc03 marked 2 inline comments as done.Aug 15 2023, 5:33 AM
nikic accepted this revision.Aug 15 2023, 5:34 AM

LGTM

This revision is now accepted and ready to land.Aug 15 2023, 5:34 AM
This revision was landed with ongoing or failed builds.Aug 15 2023, 10:07 AM
This revision was automatically updated to reflect the committed changes.