Page MenuHomePhabricator

Have a way to let fast-isel handle normally optimized library functions
Needs ReviewPublic

Authored by rkotler on Feb 3 2015, 6:02 PM.

Details

Reviewers
echristo
Summary

For some reason, LLVM decides to not handle functions in the C library that are normally
marked as optimized. I don't know the history of this but for sure in the mips fast-isel port we don't
want this behavior so I have provided a way for a class that is derived from the target independent fast-isel
to override this behavior.

Diff Detail

Event Timeline

rkotler updated this revision to Diff 19295.Feb 3 2015, 6:02 PM
rkotler retitled this revision from to Have a way to let fast-isel handle normally optimized library functions.
rkotler updated this object.
rkotler edited the test plan for this revision. (Show Details)
rkotler added a reviewer: mcrosier.
rkotler added subscribers: dsanders, rfuhler, Unknown Object (MLST).
rkotler edited the test plan for this revision. (Show Details)Feb 3 2015, 8:18 PM

If we're going to do this we should just do it unconditionally and add support to the various backends. Can you provide the principle behind the change?

-eric

I don't know what the current logic is behind having fast-isel not treat these C library as ordinary functions so I'm not prepared to force what I want on other ports. It clearly makes no sense in the MIps port because the result is that fast-isel will quit there and for Mips we are trying to minimize this behavior so that the code generation runs faster.

In method: void SelectionDAGISel::SelectAllBasicBlocks(const Function &Fn) , in SelectionDAGISel.cpp it asks the question if (FastIS->selectInstruction(Inst)) { .... if the answer is false then it will proceed to

// Then handle certain instructions as single-LLVM-Instruction blocks.
 if (isa<CallInst>(Inst)) {

   if (EnableFastISelVerbose || EnableFastISelAbort) {
     dbgs() << "FastISel missed call: ";
     Inst->dump();
   }

and hence the call is missed.

In method bool FastISel::selectInstruction(const Instruction *I) is where the logic is that I'm changing.
Someone has made a conscious choice, which I assume also has "make check" tests that will fail otherwise,
to not treat these builtin C library functions as ordinary functions and hence make fast-isel fail here and
revert to non fast-isel.

Anyone that wants the behavior I want for mips fast-isel can just turn this on as I have in the constructor
for their fast isel:

HandleOptimizedCodeGenFunctions(false)

If everyone decides to do that then we could change the original code but I'm not prepared to submit a patch
that makes this decision for other target ports.

Correction: the override code placed in the derived fast-isel constructor is:

setHandleOptimizedCodeGenFunctions(true);

echristo edited edge metadata.Feb 4 2015, 4:40 PM

The idea was that you would look at the commits for the existing lines of
code. I know the general motivation.

-eric

This original code was added by Bob Wilson.

The following commit from the commit:
d49edb7ab098fa0c82f59efbcf1b4eb2958f8dc3

Fall back to selection DAG isel for calls to builtin functions.

Fast isel doesn't currently have support for translating builtin function
calls to target instructions. For embedded environments where the library
functions are not available, this is a matter of correctness and not
just optimization. Most of this patch is just arranging to make the
TargetLibraryInfo available in fast isel. rdar://problem/12008746

The problem with this approach is that the fast-isel code fro a given target can always for things to fall back to selection dag but this patch forces it to do that in all cases.

The problem with this approach is that the fast-isel code for a given target can always force things to fall back to selection dag but this patch forces it to do that in all cases.

( http://llvm.org/klaus/llvm/commit/d49edb7ab098fa0c82f59efbcf1b4eb2958f8dc3/ )

In a later patch,

24b3fc7 (Juergen Ributzka 2014-07-11 22:01:42 +0000 1594)
60420346 (Juergen Ributzka 2014-09-03 20:56:52 +0000 1595) bool FastISel::fastLowerIntrinsicCall(const IntrinsicInst * /*II*/) {
2f58a513 (Juergen Ributzka 2014-07-11 20:42:12 +0000 1596) return false;
2f58a513 (Juergen Ributzka 2014-07-11 20:42:12 +0000 1597) }

fastLowerInstrinsicCall is broken out separately so now it's easy for the target dependent code to decide which
ones they want to handle or not.

For example:

bool MipsFastISel::fastLowerIntrinsicCall(const IntrinsicInst *II) {

switch (II->getIntrinsicID()) {
default:
  return false;

case Intrinsic::bswap: {

Type *RetTy = II->getCalledFunction()->getReturnType();

.....

Note that this hasn't fallen off my radar. I'll try to get to it tomorrow.

It seems from this history of this problem that we could just delete the
code for all targets that my patch works around.

It seems it was just a bandaid on another problem that was better solved
by just adding a virtual method in fast-isel
for handling intrinsics.

Now that that is there, any intrinsic that a given port does not want to
handle will get handled by reverting
to non fast-isel.

But I would still like to apply my patch first while we investigate that
there is no unwanted ramifications
that would affect other ports.

Hi Reed,

Not such a huge fan of this patch, I'd rather just do this independently by having the selection mechanism just handle the functions. How were you planning on implementing aspects of this and why wouldn't it just work in general?

Thanks!
-eric

The normal selection mechanism would work fine but current hack in
fast-isel prevents it.

The selection mechanism should call the FastInstrinsciCall to do this.

Then it would be up to the individual port whether to ignore it.

So we could delete the following lines from selectInstruction:

// As a special case, don't handle calls to builtin library functions that
// may be translated directly to target instructions.
if (F && !F->hasLocalLinkage() && F->hasName() &&
    LibInfo->getLibFunc(F->getName(), Func) &&
    LibInfo->hasOptimizedCodeGen(Func))
  return false;