[llvm-dev] [RFC] IRBuilder polymorphism: Templates/virtual

[Adrien Guinet via llvm-dev]

On 2/5/20 10:56 PM, Nikita Popov via llvm-dev wrote:
> Hi,
> 
> The IRBuilder is currently templated over a constant folder, and an
> instruction inserter. https://reviews.llvm.org/D73835 proposes to move
> this towards using virtual dispatch instead. As this is a larger design
> change, I would like to get some feedback on this.
> 
> The current templated design of IRBuilder has a couple of problems:
> 1. It's not possible to share code between use-sites that use different
> IRBuilder folders/inserters (short of templating the code and moving it
> into headers).
> 2. Methods currently defined on IRBuilderBase (which is not templated)
> do not use the custom inserter, resulting in subtle bugs (e.g. incorrect
> InstCombine worklist management). It would be possible to move those
> into the templated IRBuilder, but...
> 3. The vast majority of the IRBuilder implementation has to live in the
> header, because it depends on the template arguments.
> 4. We have many unnecessary dependencies on IRBuilder.h, because it is
> not easy to forward-declare. (Significant parts of the backend depend on
> it via TargetLowering.h, for example.)
> 
> The referenced patch makes the constant folder and instruction inserter
> use virtual dispatch instead. IRBuilder remains templated, but is only a
> thin wrapper around IRBuilderBase, which contains all the logic and
> creation methods. Functions using the IR builder only need to accept
> IRBuilderBase&, and headers can forward-declare that type.
> 
> The disadvantage of the change is that additional virtual dispatch may
> make the IRBuilder a bit more expensive. Moving the implementation of
> IRBuilder methods from the header into the cpp file (not direct part of
> the proposed change, but a natural followup it enables) would further
> limit inlining opportunities.
> 
> What are your thoughts on this?

IMHO, all the reasons you mention are valid ones.

If you have time, maybe a "before/after" benchmark on a pass that
extensively uses IRBuilder, on a code that would trigger it a lot (I
don't have anything in mind right now).