[PATCH] D73835: [IRBuilder] Virtualize IRBuilder

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

In D73835#1857028 <https://reviews.llvm.org/D73835#1857028>, @Meinersbur wrote:

> In my experience, LLVM tends more towards removing virtual inheritance and towards compile-time polymorphism (e.g. the pass manager, llvm::Instruction without vtable). This is a patch into the other direction. Personally, I prefer to use the language-provided mechanism.
>
> There might be a performance impact (less inlining), it would help if you could measure it.


Unfortunately I don't have a setup suitable to perform compile-time benchmarks. I can barely get LLVM built, my previous attempts at compile-time measurements were hopelessly noisy. I can't really imagine IRBuilder taking up a large fraction of compile-time though (I don't believe I've ever seen it show up in profiles.)

I'm not particularly good with C++, so if there's some other way to go about this that avoids using "virtual", I'd be happy to try that. I think that disregarding all the other considerations, the one issue here that must be fixed one way or another is that the "inserter" is not used if methods defined on IRBuilderBase are called. In particular this means that any intrinsics inserted by InstCombine right now do not get added to the worklist. Other passes that use a custom inserter probably experience similar subtle issues.

It might make sense to break this patch up to address just add that issue first, and leave the more involved "constant folder" changes to another patch. That would still require making something virtual though... either by making the inserter virtual (what this patch does), or making IRBuilder::Insert virtual (https://gist.github.com/nikic/7d6223bc0df4798181fa781ca771e78c for a quick prototype). I actually like the latter a bit more (it is less intrusive), but this approach doesn't allow us to perform vtable anchoring (I don't know how much we care about that). Another possibility would be to just add an `std::function` callback on `IRBuilderBase` -- which is essentially the same as using "virtual", but leaves us in control of the implementation details.


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

https://reviews.llvm.org/D73835