[PATCH] D85257: [X86] Optimize getImpliedDisabledFeatures & getImpliedEnabledFeatures

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added a comment.

4c9ed3ed3d2fc7622acf5fc0d80ad20b44cf376a <https://reviews.llvm.org/rG4c9ed3ed3d2fc7622acf5fc0d80ad20b44cf376a> :
Elapsed (wall clock) time (h:mm:ss or m:ss): 0:56.05
Top methods in bottom up trace:

  +    9.17%  clang-12                   [.] getImpliedDisabledFeatures
  -    3.04%  clang-12                   [.] llvm::StringMapImpl::LookupBucketFor
     - 3.01% llvm::StringMapImpl::LookupBucketFor
        - 1.10% llvm::StringMap<bool, llvm::MallocAllocator>::try_emplace<>
           + 1.10% clang::targets::X86TargetInfo::setFeatureEnabled
  +    1.67%  clang-12                   [.] llvm::X86::getImpliedFeatures

0c7af8c83bd1acb0ca78f35ddde29b6fde4363a0 <https://reviews.llvm.org/rG0c7af8c83bd1acb0ca78f35ddde29b6fde4363a0>:
Elapsed (wall clock) time (h:mm:ss or m:ss): 0:53.39
Top methods in bottom up trace:

  +    3.59%  clang-12                   [.] llvm::X86::getImpliedFeatures
  -    3.36%  clang-12                   [.] llvm::StringMapImpl::LookupBucketFor
     - 3.32% llvm::StringMapImpl::LookupBucketFor
        - 1.24% llvm::StringMap<bool, llvm::MallocAllocator>::try_emplace<>
           + 1.24% clang::targets::X86TargetInfo::setFeatureEnabled

So that's good; `getImpliedDisabledFeatures` now falls out of the top slot.  Looks like memoizing `llvm::X86::getImpliedFeatures` will still help, a lot.

In D85257#2195135 <https://reviews.llvm.org/D85257#2195135>, @MaskRay wrote:

> In D85257#2195075 <https://reviews.llvm.org/D85257#2195075>, @nickdesaulniers wrote:
>
>> I'll try to get some numbers comparing overall build time, and full build profiles with this applied, tomorrow. Thanks for the hard work! (And I'll work with @Nathan-Huckleberry on the suggestion for `Sema::ActOnGCCAsmStmt`.  We can likely save a pointer to the `llvm::StringMap<bool> FeatureMap` within `Sema` and only recompute it if we have a `target` function attribute).
>
> You may check whether there is improvement room. I'd be wary of creating a cache as latent caching invalidation bugs would be difficult to debug :(

But I don't expect that the "implied features" would change, so there would be no need to invalidate any cache.  Caching without invalidation is just memoization.  Or is `llvm::X86::getImpliedFeatures` not pure?  I suspect it is.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85257/new/

https://reviews.llvm.org/D85257