[llvm] r221948 - First stage of call lowering for Mips fast-isel

[Daniel Sanders]

Hi,

I've been side-tracked by some more bugs in the N32/N64 ABI's but I'm planning to move the #include of MipsGenCallingConv.inc to MipsCCState.cpp. Once that's done, we'll only have one copy of these functions. I'm also going to eliminate the use of CustomCallingConv which is the reason Fast ISel and SelectionDAG have different implementations of a couple of them (the FastISel ones are stubs containing llvm_unreachable()).

From: llvm-commits-bounces at cs.uiuc.edu [mailto:llvm-commits-bounces at cs.uiuc.edu] On Behalf Of Eric Christopher
Sent: 14 November 2014 21:19
To: Reid Kleckner; Reed Kotler
Cc: llvm-commits
Subject: Re: [llvm] r221948 - First stage of call lowering for Mips fast-isel


On Thu Nov 13 2014 at 5:36:40 PM Reid Kleckner <rnk at google.com<mailto:rnk at google.com>> wrote:
On Thu, Nov 13, 2014 at 5:24 PM, reed kotler <rkotler at mips.com<mailto:rkotler at mips.com>> wrote:
On 11/13/2014 05:20 PM, Reid Kleckner wrote:
Looks like you're including that .inc file twice, which is going to duplicate the code. IMO it'd be better to call over from FastISel into a wrapper in ISelLowering if you want to use these.

I think it's a mistake for tablegen to be generating static functions here.

Why should I have to make a wrapper?

I've raised this issue before but did not get any response.

Wow. Looks like all the other backends duplicate this code:

$ git grep '#include.*GenCallingConv.inc' ../lib/Target/
../lib/Target/AArch64/AArch64FastISel.cpp:#include "AArch64GenCallingConv.inc"
../lib/Target/AArch64/AArch64ISelLowering.cpp:#include "AArch64GenCallingConv.inc"
../lib/Target/ARM/ARMFastISel.cpp:#include "ARMGenCallingConv.inc"
../lib/Target/ARM/ARMISelLowering.cpp:#include "ARMGenCallingConv.inc"
../lib/Target/MSP430/MSP430ISelLowering.cpp:#include "MSP430GenCallingConv.inc"
../lib/Target/Mips/MipsFastISel.cpp:#include "MipsGenCallingConv.inc"
../lib/Target/Mips/MipsISelLowering.cpp:#include "MipsGenCallingConv.inc"
../lib/Target/PowerPC/PPCFastISel.cpp:#include "PPCGenCallingConv.inc"
../lib/Target/PowerPC/PPCISelLowering.cpp:#include "PPCGenCallingConv.inc"
../lib/Target/R600/AMDGPUISelLowering.cpp:#include "AMDGPUGenCallingConv.inc"
../lib/Target/Sparc/SparcISelLowering.cpp:#include "SparcGenCallingConv.inc"
../lib/Target/SystemZ/SystemZISelLowering.cpp:#include "SystemZGenCallingConv.inc"
../lib/Target/X86/X86FastISel.cpp:#include "X86GenCallingConv.inc"
../lib/Target/X86/X86ISelLowering.cpp:#include "X86GenCallingConv.inc"
../lib/Target/XCore/XCoreISelLowering.cpp:#include "XCoreGenCallingConv.inc"

This seems less than ideal. Eric, should we really be doing that?

Ideally no. I mean, we could generate separate CC.inc files for fast-isel? A wrapper seems reasonable as well, but seems like a shame we can't generate the right stuff out of it.

Perhaps the best solution is taking the calling convention bits from being a bunch of static functions in a namespace and encapsulating them into a class in a header that can be extended by the target. I.e. giving it a wrapper by default?

That's probably what I'd do.

-eric
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141117/33e9c7a3/attachment.html>