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

[rkotler]

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.

[echristo]

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

[rkotler]

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.

[rkotler]

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

setHandleOptimizedCodeGenFunctions(true);

[echristo]

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

-eric

[rkotler]

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.

[rkotler]

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

[rkotler]

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/ )

[rkotler]

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();

.....

[echristo]

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

[rkotler]

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.

[rkotler]

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

[echristo]

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

[rkotler]

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;

include/llvm/CodeGen/FastISel.h

protected:
   const TargetMachine &TM;
   const DataLayout &DL;
   const TargetInstrInfo &TII;
   const TargetLowering &TLI;
   const TargetRegisterInfo &TRI;
   const TargetLibraryInfo *LibInfo;
   bool SkipTargetIndependentISel;
 
+  bool HandleOptimizedCodeGenFunctions;
+
   /// \brief The position of the last instruction for materializing constants
   /// for use in the current block. It resets to EmitStartPt when it makes sense
   /// (for example, it's usually profitable to avoid function calls between the
   /// definition and the use)
   MachineInstr *LastLocalValue;
 
   /// \brief The top most instruction in the current block that is allowed for
   /// emitting local variables. LastLocalValue resets to EmitStartPt when it
protected:
   bool selectPatchpoint(const CallInst *I);
   bool selectCall(const User *Call);
   bool selectIntrinsicCall(const IntrinsicInst *II);
   bool selectBitCast(const User *I);
   bool selectCast(const User *I, unsigned Opcode);
   bool selectExtractValue(const User *I);
   bool selectInsertValue(const User *I);
 
+  void setHandleOptimizedCodeGenFunctions(bool Value);
+  bool handleOptimizedCodeGenFunctions();
+
 private:
   /// \brief Handle PHI nodes in successor blocks.
   ///
   /// Emit code to ensure constants are copied into registers when needed.
   /// Remember the virtual registers that need to be added to the Machine PHI
   /// nodes as input.  We cannot just directly add them, because expansion might
   /// result in multiple MBB's for one BB.  As such, the start of the BB might
   /// correspond to a different MBB than the end.

lib/CodeGen/SelectionDAG/FastISel.cpp

bool FastISel::selectInstruction(const Instruction *I) {
   SavedInsertPt = FuncInfo.InsertPt;
 
   if (const auto *Call = dyn_cast<CallInst>(I)) {
     const Function *F = Call->getCalledFunction();
     LibFunc::Func Func;
 
     // 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() &&
+    if (!handleOptimizedCodeGenFunctions() &&
+        F && !F->hasLocalLinkage() && F->hasName() &&
         LibInfo->getLibFunc(F->getName(), Func) &&
         LibInfo->hasOptimizedCodeGen(Func))
       return false;
 
     // Don't handle Intrinsic::trap if a trap funciton is specified.
     if (F && F->getIntrinsicID() == Intrinsic::trap &&
         !TM.Options.getTrapFunctionName().empty())
       return false;
FastISel::FastISel(FunctionLoweringInfo &FuncInfo,
                    const TargetLibraryInfo *LibInfo,
                    bool SkipTargetIndependentISel)
     : FuncInfo(FuncInfo), MF(FuncInfo.MF), MRI(FuncInfo.MF->getRegInfo()),
       MFI(*FuncInfo.MF->getFrameInfo()), MCP(*FuncInfo.MF->getConstantPool()),
       TM(FuncInfo.MF->getTarget()), DL(*TM.getDataLayout()),
       TII(*MF->getSubtarget().getInstrInfo()),
       TLI(*MF->getSubtarget().getTargetLowering()),
       TRI(*MF->getSubtarget().getRegisterInfo()), LibInfo(LibInfo),
-      SkipTargetIndependentISel(SkipTargetIndependentISel) {}
+      SkipTargetIndependentISel(SkipTargetIndependentISel),
+      HandleOptimizedCodeGenFunctions(false) {}
 
 FastISel::~FastISel() {}
 
 bool FastISel::fastLowerArguments() { return false; }
 
 bool FastISel::fastLowerCall(CallLoweringInfo & /*CLI*/) { return false; }
 
 bool FastISel::fastLowerIntrinsicCall(const IntrinsicInst * /*II*/) {
CmpInst::Predicate FastISel::optimizeCmpPredicate(const CmpInst *CI) const {
   case CmpInst::ICMP_SGT:   Predicate = CmpInst::FCMP_FALSE; break;
   case CmpInst::ICMP_SGE:   Predicate = CmpInst::FCMP_TRUE;  break;
   case CmpInst::ICMP_SLT:   Predicate = CmpInst::FCMP_FALSE; break;
   case CmpInst::ICMP_SLE:   Predicate = CmpInst::FCMP_TRUE;  break;
   }
 
   return Predicate;
 }
+
+void FastISel::setHandleOptimizedCodeGenFunctions(bool Value) {
+  HandleOptimizedCodeGenFunctions = Value;
+}
+
+bool FastISel::handleOptimizedCodeGenFunctions() {
+  return HandleOptimizedCodeGenFunctions;
+}

lib/Target/Mips/MipsFastISel.cpp

explicit MipsFastISel(FunctionLoweringInfo &funcInfo,
         TII(*Subtarget->getInstrInfo()), TLI(*Subtarget->getTargetLowering()) {
     MFI = funcInfo.MF->getInfo<MipsFunctionInfo>();
     Context = &funcInfo.Fn->getContext();
     TargetSupported =
         ((TM.getRelocationModel() == Reloc::PIC_) &&
          ((Subtarget->hasMips32r2() || Subtarget->hasMips32()) &&
           (static_cast<const MipsTargetMachine &>(TM).getABI().IsO32())));
     UnsupportedFPMode = Subtarget->isFP64bit();
+    setHandleOptimizedCodeGenFunctions(true);
   }
 
   unsigned fastMaterializeConstant(const Constant *C) override;
   bool fastSelectInstruction(const Instruction *I) override;
 
 #include "MipsGenFastISel.inc"
 };
 } // end anonymous namespace.

test/CodeGen/Mips/Fast-ISel/strlen2.ll

+; RUN: llc -march=mipsel -relocation-model=pic -O0 -fast-isel -mips-fast-isel -fast-isel-abort -mcpu=mips32r2 \
+; RUN:     < %s | FileCheck %s
+
+@.str = private unnamed_addr constant [6 x i8] c"hello\00", align 1
+@hello = global i8* getelementptr inbounds ([6 x i8]* @.str, i32 0, i32 0), align 4
+@len = common global i32 0, align 4
+
+; Function Attrs: nounwind
+define void @foo()  {
+entry:
+; CHECK:  .ent  foo
+  %0 = load i8** @hello, align 4
+  %call = call i32 @strlen(i8* %0) #2
+  store i32 %call, i32* @len, align 4
+  ret void
+}
+
+; Function Attrs: nounwind readonly
+declare i32 @strlen(i8*) 
+
+