Cleaning up ARM parts of the Clang driver

Hi,

I’m doing some cleaning up of the target-specific hacks in the Driver, as prep for some cross-compilation changes discussed previously on the list.

My previous set of patches tablegen’d up a lot of the horrible string matching, but I’ve realised that actually it’s almost all superfluous and can be deleted completely.

That said, there are some defaults and corner cases that I’d like the list’s (and especially Douglas, Chandler and Eric’s) opinions on before I go any further. This all relates to the ARM specific stuff – I can’t really touch the Intel, MIPS or PowerPC stuff so it’d be better if someone else cleans that up if they feel like it. A lot of these changes revolve around not trying to be too clever in the frontend driver and leaving defaults to more specialised parts of the back ends.

Does anyone have any opinions or answers to the following? Thanks!

1:

// Select the ABI to use.

//

// FIXME: Support -meabi.

const char *ABIName = 0;

if (Arg *A = Args.getLastArg(options::OPT_mabi_EQ)) {

ABIName = A->getValue(Args);

} else {

// Select the default based on the platform.

switch(Triple.getEnvironment()) {

case llvm::Triple::GNUEABI:

ABIName = “aapcs-linux”;

break;

case llvm::Triple::EABI:

ABIName = “aapcs”;

break;

default:

ABIName = “apcs-gnu”;

}

}

CmdArgs.push_back("-target-abi");

CmdArgs.push_back(ABIName);

I don’t really think the target ABI needs to be set if it is part of the triple (the else clause) – that should be inferred by all other tools by the triple.

2: The driver currently tries to pick a default CPU for the architecture if none is specified. I think this is wrong – if no CPU is specified then the compiler should not tune for a specific CPU. The user would expect if specifying –arch armv7, for the compiler to give an output that has good blended performance on all armv7 cores, not cortex-a8 specifically for example.

3: Selecting the floating point ABI is a rather large set of switch-cases which I think can be contracted into a simple:

// If unspecified, choose the default based on the platform.

if (FloatABI.empty()) {

const llvm::Triple &Triple = getToolChain().getTriple();

if (Triple.getOS() == llvm::Triple::Darwin &&

(ArchName.startswith(“v6”) || ArchName.startswith(“v7”))

FloatABI = “soft”;

else

FloatABI = “softfp”;

}

I also don’t see why the current default is “soft” if no ABI is specified. Why not “softfp”? If the target has hardware FP support that’ll be used, if not then it is implied that the ABI is “soft”. I also don’t know how critical that Darwin special-case is. If it could be gotten rid of, that’d be lovely.

4: These are hacks:

// Use software floating point operations?

if (FloatABI == “soft”) {

CmdArgs.push_back("-target-feature");

CmdArgs.push_back("+soft-float");

}

// Use software floating point argument passing?

if (FloatABI != “hard”) {

CmdArgs.push_back("-target-feature");

CmdArgs.push_back("+soft-float-abi");

}

They’re referenced from lib/Basic/Targets.cpp – would it not be better for Targets.cpp to infer these from the other passed parameters? That’s actually a self-answering question, so my question really is: “Is there any reason why I can’t make the trivial change to make this so?”

5:

// Set the target features based on the FPU.

if (FPU == “fpa” || FPU == “fpe2” || FPU == “fpe3” || FPU == “maverick”) {

Where did these names come from? Do people actually use them? I see google results for GCC command lines specific to a Cirrus core – is that the reason they’re there? Can we get rid of them as nasty GCC relics?

6:

// Setting -msoft-float effectively disables NEON because of the GCC

// implementation, although the same isn’t true of VFP or VFP3.

if (FloatABI == “soft”) {

CmdArgs.push_back("-target-feature");

CmdArgs.push_back("-neon");

}

It’s not evident why this code is here. Why does –msoft-float disable NEON? NEON has an integer vectoriser too. GCC relic?

7:

// Setting -mno-global-merge disables the codegen global merge pass. Setting

// -mglobal-merge has no effect as the pass is enabled by default.

if (Arg *A = Args.getLastArg(options::OPT_mglobal_merge,

options::OPT_mno_global_merge)) {

if (A->getOption().matches(options::OPT_mno_global_merge))

CmdArgs.push_back("-mno-global-merge");

}

It seems like this should be better handled by the sys::cl code in llc.

Cheers,

James