Teach FastISel about thiscall (and, hence, about callee-pop).

[thakis]

Summary

Without this, -O0 builds fail to use fast isel on all member functions in 32-bit Windows compiles: Fast ISel walks from back to front, and each ret on a member function uses thiscall.

Even with this patch, fast isel fails to lower thiscall calls, and it never fires in 64-bit Windows compiles, since those currently have an explicit early return too. Chrome/mac debug builds of the v8_base target get ~6% slower if I disable fast isel there, so it might be worth trying to get it to fire more often in Windows debug builds.

[rnk]

Can we test the case when the parameters are greater than 64KB? Is there some other example test that deals with the cases where we bail out?

[thakis]

add test

[thakis]

In D22115#477460, @rnk wrote:

Can we test the case when the parameters are greater than 64KB? Is there some other example test that deals with the cases where we bail out?

Added a test for the 64kB, based on x86-big-ret.ll. (I'm not sure I understand what you mean with the second question -- do you want more tests, or do you mean as example for how to do the 64kB test?)

[rnk]

In D22115#477488, @thakis wrote:

In D22115#477460, @rnk wrote:

Can we test the case when the parameters are greater than 64KB? Is there some other example test that deals with the cases where we bail out?

Added a test for the 64kB, based on x86-big-ret.ll. (I'm not sure I understand what you mean with the second question -- do you want more tests, or do you mean as example for how to do the 64kB test?)

I'm not sure what the best way is to test fast isel fallback, so I was asking if there are similar tests nearby that do it well. A lot of fastisel tests use -fast-isel-abort so that they fail if we stop being able to select simple code sequences.

[thakis]

I looked through a few recent changes to X86FastISel.cpp. Most don't test the early-out branches at all. The ones that fix bugs with commit messages like "bail out to SelectionDAG since it gets this right" usually check that the right code is generated, like the new test I added now does here.

(ps: It's a bit weird that retl $65536 silently assembles to ret 0)

[rnk]

In D22115#477669, @thakis wrote:

I looked through a few recent changes to X86FastISel.cpp. Most don't test the early-out branches at all. The ones that fix bugs with commit messages like "bail out to SelectionDAG since it gets this right" usually check that the right code is generated, like the new test I added now does here.

Sounds good

(ps: It's a bit weird that retl $65536 silently assembles to ret 0)

sad trombone :(

[thakis]

r275135, thanks!

lib/Target/X86/X86FastISel.cpp

bool X86FastISel::X86SelectRet(const Instruction *I) {
 
   if (TLI.supportSplitCSR(FuncInfo.MF))
     return false;
 
   CallingConv::ID CC = F.getCallingConv();
   if (CC != CallingConv::C &&
       CC != CallingConv::Fast &&
       CC != CallingConv::X86_FastCall &&
+      CC != CallingConv::X86_ThisCall &&
       CC != CallingConv::X86_64_SysV)
     return false;
 
   if (Subtarget->isCallingConvWin64(CC))
     return false;
 
-  // Don't handle popping bytes on return for now.
-  if (X86MFInfo->getBytesToPopOnReturn() != 0)
+  // Don't handle popping bytes if they don't fit the ret's immediate.
+  if (!isUInt<16>(X86MFInfo->getBytesToPopOnReturn()))
     return false;
 
   // fastcc with -tailcallopt is intended to provide a guaranteed
   // tail call optimization. Fastisel doesn't know how to do that.
   if (CC == CallingConv::Fast && TM.Options.GuaranteedTailCallOpt)
     return false;
 
   // Let SDISel handle vararg functions.
assert(Reg &&
            "SRetReturnReg should have been set in LowerFormalArguments()!");
     unsigned RetReg = Subtarget->is64Bit() ? X86::RAX : X86::EAX;
     BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc,
             TII.get(TargetOpcode::COPY), RetReg).addReg(Reg);
     RetRegs.push_back(RetReg);
   }
 
   // Now emit the RET.
-  MachineInstrBuilder MIB =
-    BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc,
+  MachineInstrBuilder MIB;
+  if (X86MFInfo->getBytesToPopOnReturn()) {
+    MIB = BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc,
+                  TII.get(Subtarget->is64Bit() ? X86::RETIQ : X86::RETIL))
+              .addImm(X86MFInfo->getBytesToPopOnReturn());
+  } else {
+    MIB = BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc,
                   TII.get(Subtarget->is64Bit() ? X86::RETQ : X86::RETL));
+  }
   for (unsigned i = 0, e = RetRegs.size(); i != e; ++i)
     MIB.addReg(RetRegs[i], RegState::Implicit);
   return true;
 }
 
 /// X86SelectLoad - Select and emit code to implement load instructions.
 ///
 bool X86FastISel::X86SelectLoad(const Instruction *I) {

test/CodeGen/X86/fast-isel-x86.ll

 ; This should pop 4 bytes on return.
 ; CHECK-LABEL: test1:
 ; CHECK: retl $4
 define void @test1({i32, i32, i32, i32}* sret %p) nounwind {
   store {i32, i32, i32, i32} zeroinitializer, {i32, i32, i32, i32}* %p
   ret void
 }
 
+; This should pop 8 bytes on return.
+; CHECK-LABEL: thiscallfun:
+; CHECK: retl $8
+define x86_thiscallcc void @thiscallfun(i32* %this, i32 %a, i32 %b) nounwind {
+  ret void
+}
+
+; Here, the callee pop doesn't fit the 16 bit immediate -- see x86-big-ret.ll
+; This checks that -fast-isel doesn't miscompile this.
+; CHECK-LABEL: thiscall_large:
+; CHECK:      popl %ecx
+; CHECK-NEXT: addl $65536, %esp
+; CHECK-NEXT: pushl %ecx
+; CHECK-NEXT: retl
+define x86_thiscallcc void @thiscall_large(i32* %this, [65533 x i8]* byval %b) nounwind {
+  ret void
+}
+
 ; Properly initialize the pic base.
 ; CHECK-LABEL: test2:
 ; CHECK-NOT: HHH
-; CHECK: call{{.*}}L2$pb
-; CHECK-NEXT: L2$pb:
+; CHECK: call{{.*}}L4$pb
+; CHECK-NEXT: L4$pb:
 ; CHECK-NEXT: pop
 ; CHECK: HHH
 ; CHECK: retl
 @HHH = external global i32
 define i32 @test2() nounwind {
   %t = load i32, i32* @HHH
   ret i32 %t
 }

 ; CHECK-LABEL: test5:
 ; Local value area is still there:
 ; CHECK: movl $42, {{%[a-z]+}}
 ; Fast-ISel's arg push is not here:
 ; CHECK-NOT: movl $42, (%esp)
 ; SDag-ISel's arg push:
 ; CHECK: movl %esp, [[REGISTER:%[a-z]+]]
 ; CHECK: movl $42, ([[REGISTER]])
-; CHECK: movl L_test5dllimport$non_lazy_ptr-L5$pb(%eax), %eax
+; CHECK: movl L_test5dllimport$non_lazy_ptr-L7$pb(%eax), %eax
 
 }
 declare dllimport i32 @test5dllimport(i32)