Proposal to merge SimplifyLibCalls into InstCombiner

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.

Abstract

call-folding-merge-proposal.patch (15.8 KB)

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 :slight_smile:

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

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.

Yup, that's it.

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 :slight_smile:

Do you think we still need some way to disable just the library call
simplification now that it is a part of the instruction combiner?

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.

Thanks for the explanation. As a part of the merger I really want
`SimplifyFortifiedLibCalls` to go away as well, since the fortified
library call folding is being rolled into `LibCallSimplifier`. I will
look at applying `LibCallSimplifier` to the CGP case as well (the conditions
for when it is OK to fold is slightly different for CGP).

Thanks for the feedback!

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 :slight_smile:

Do you think we still need some way to disable just the library call
simplification now that it is a part of the instruction combiner?

It should just be part of instcombine, but be gated by what TargetLibraryInfo says. If you want to turn off these libcall optimizations, it should be by marking the libcalls as off-limits in TLI.

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.

Thanks for the explanation. As a part of the merger I really want
`SimplifyFortifiedLibCalls` to go away as well, since the fortified
library call folding is being rolled into `LibCallSimplifier`. I will
look at applying `LibCallSimplifier` to the CGP case as well (the conditions
for when it is OK to fold is slightly different for CGP).

Ok, please just tackle this as a follow-on step. Thanks Meador!

-Chris

There is also the matter of -fno-builtin, which I missed in my original
proposal. Patch posted here:
http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20121105/155370.html.