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
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
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.
1. What should be done about the `SimplifyFortifiedLibCalls` use in
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.