[PATCH] D73835: [IRBuilder] Virtualize IRBuilder

Tue Feb 4 13:46:42 PST 2020

Meinersbur added a comment.

In D73835#1857727 <https://reviews.llvm.org/D73835#1857727>, @nikic wrote:

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

It doesn't need to be an entire run of clang. Even a result 'difference is not significant' behind the noise is some supporting result over guessing.

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

The obvious solution would be to move these methods from IRBuilderBase to IRBuilder.

This is a significant change for the framework, I'd recommend writing an RFC to the llvm-dev list to get more feedback.

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

https://reviews.llvm.org/D73835