[PATCH] D149855: [DAGCombiner] Avoid template for generalized pattern match.

[Simon Moll via Phabricator via llvm-commits]

simoll added a comment.

In D149855#4327203 <https://reviews.llvm.org/D149855#4327203>, @craig.topper wrote:

> In D149855#4326180 <https://reviews.llvm.org/D149855#4326180>, @simoll wrote:
>
>>> There is a concern that using template for many functions in
>>> DAGCombiner.cpp may make the binary too large.
>>
>> Could you provide more details on this concern, eg where was it raised? To put this discussion in perspective, looking at vanilla LLVM 14, a debug build of `DAGCombiner.cpp.o` has `7MB` vs `1322MB` for `libLLVM-14.so` (with `X86`,`RISCV`). I would add that this approach adds virtual dispatch on the standard DAGCombiner path, which has a performance cost. IMHO, the question is what a good tradeoff would be.
>
> I raised it an internal SiFive discussion. I agree a virtual dispatch and a heap allocation are not an improvement. We haven't applied this generic matcher to much code yet and if we started applying it aggressively we may double the size of DAGCombiner.cpp.o. So I thought it was worth thinking about alternative abstractions that didn't duplicate entire functions. I just don't have any good ideas yet.

You don't need to rewrite the templated code to have the option for virtual dispatch in the future:
If the need arises, we could implement one `SuperMatchContext` that defers to `EmptyContext` and `VPMatchContext` internally (via subclasses or otherwise) and only instantiate the templates for `SuperMatchContext` once.
(If you go down this route, there is a bunch of cool things you can do, eg combining different matchers or running matcher code with multiple contexts in parallel).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149855