[PATCH] D112175: [NFC] Add llvm::StaticVector ADT

Thu May 5 10:37:16 PDT 2022

dblaikie added a comment.

In D112175#3489258 <https://reviews.llvm.org/D112175#3489258>, @dexonsmith wrote:

> In D112175#3489247 <https://reviews.llvm.org/D112175#3489247>, @dexonsmith wrote:
>
>> Thanks for running that. I just pushed https://github.com/dexonsmith/llvm-project/tree/perf/small-vector-specialize-reserveForParamAndGetAddressImpl, which should cause compile-time results to show up at http://llvm-compile-time-tracker.com/index.php?config=NewPM-O3&stat=instructions&remote=dexonsmith eventually, and we can see if there's a meaningful impact outside of the benchmark. Not sure whether it's worth changing `push_back()` since even on the toy benchmark it's not a big difference.
>
> Also, it'd be interesting to look at what is different about the CodeGen, and hunt down why the optimizations are missed. Seems better to fix the optimizer than to add redundant specializations all over the place.

Yeah, it seems unfortunate that that produces any different code with or without the specialization, given it's one trivial/compile-time-constant condition & then a follow-on conditional operator.

In D112175#3492649 <https://reviews.llvm.org/D112175#3492649>, @dexonsmith wrote:

> In D112175#3489247 <https://reviews.llvm.org/D112175#3489247>, @dexonsmith wrote:
>
>> Thanks for running that. I just pushed https://github.com/dexonsmith/llvm-project/tree/perf/small-vector-specialize-reserveForParamAndGetAddressImpl, which should cause compile-time results to show up at http://llvm-compile-time-tracker.com/index.php?config=NewPM-O3&stat=instructions&remote=dexonsmith eventually, and we can see if there's a meaningful impact outside of the benchmark. Not sure whether it's worth changing `push_back()` since even on the toy benchmark it's not a big difference.
>
> Results here:
> http://llvm-compile-time-tracker.com/compare.php?from=c1e17c7dfedd27b95c8c2fba2b6473c7348f0e77&to=0be905f5dec8ebc4bd0fc2ec498db88a617884fc&stat=instructions
>
> Almost within the noise. One thing is "light green". @dblaikie , thoughts on whether it's worth landing? (I can send a Phab review)

My gut would be probably not worth it - but if it's important to you/someone I could live with it. Though, yeah, would prefer to see the long-term fix, at least a comment near the specialization pointing to a bug that, once resolved, could then motivate removing the specialization agian.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112175