[LLVMdev] Proposal to merge SimplifyLibCalls into InstCombiner

[Chris Lattner]

On Aug 1, 2012, at 9:49 PM, Meador Inge <meadori at codesourcery.com> wrote:

> Hi All,
> 
> I finally got around to cleaning up my proposal to merge `SimplifyLibCalls`
> into `InstCombiner`.  There is still an open question or two and I am sure
> there are parts that could be better specified, but this is good enough to
> discuss.  Feedback is most welcome.

Fantastic, I'm glad that you're looking to tackle this.

> An design that combines the strength of the table-driven folder approach
> into the `InstCombiner` visitor framework is proposed.

Sounds like the right high level design.

> The following three sections specify a proposal for how the work will be broken
> down.  In a way they specify milestones.
> 
> Extend test coverage
> --------------------

+1

> Create new `LibCallSimplifier` class
> ------------------------------------
> 
> A new self-contained `LibCallSimplifier` class will be created.  An instance
> of the class will be instantiated when running the `InstCombiner` pass.  It's
> folding functionality will be invoked from `InstCombiner::tryOptimizeCall` and
> the implementation will be table-driven like `SimplifyLibCalls`.  All of the
> existing fortified library call simplifiers will be migrated to this new class
> at this step.

Ok, the idea is that other general parts of the compiler can use the LibCallSimplifier interface as well?  That makes sense to me.

> Migrate `SimplifyLibCalls` to `LibCallSimplifier`
> -------------------------------------------------
> All of the individual optimizations from `SimplifyLibCalls` will be migrated
> over to the new `LibCallSimplifier` infrastructure.  This will done
> incrementally one optimization at a time to make things easier to review.

Makes sense.  After you do the first few and people are happy with the general approach, later libcalls can be moved in larger chunks (e.g. by family) instead of each individually.

> An option for enabling/disabling library call simplification in `InstCombiner`
> will be available.  For backwards compatibility perhaps it should remain
> '-simplify-libcalls'.  The `NumSimplified` and `NumAnnotated` statistics shall
> be added to `InstCombiner`.

There is no need to preserver the -simplify-libcalls flag.  It's an internal implementation detail of the compiler, one that is better left in the past :)

> As a result of this migration 'Transforms/Scalar/SimplifyLibCalls.cpp' will go
> away.

Woot.

> Proof of Concept
> ================
> 
> A very rough proof of concept patch has been attached to this proposal
> showing how the table-driven method can be integrated into `InstCombiner`.
> This is *not* meant to be a patch submission, so please don't provide
> review points on style, implementation bugs, and other typical code review
> points.  It is merely provided to show the general direction of the
> implementation.  That being said, it does currently pass the regression
> test suite as of r161099.

Looks pretty decent to me.  You can also land the new LibCallSimplifier infrastructure first before you switch anything over to use it, if that helps.

> Open Questions
> ==============
> 
> 1. What should be done about the `SimplifyFortifiedLibCalls` use in
>   `CodeGenPrepare`?

I think it stays where it is.  CGP runs right before the code generator kicks in.  The idea here is that we want to leave the fortified libcalls in the IR as long as possible on the off chance that instcombine or something else can expose range information about underlying buffers... however, if we get all the way to code gen, then we failed to find a length, so we remove them.

-Chris