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

[Craig Topper via Phabricator via llvm-commits]

craig.topper added a comment.

In D85257#2197114 <https://reviews.llvm.org/D85257#2197114>, @nickdesaulniers wrote:

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

It's pure. But I think we should focus on getting clang to not rebuild the feature map for every inline asm statement. That will also remove a string map creation and destruction.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85257