Cleaning up ARM parts of the Clang driver

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.

You mean only pass down -target-abi when it's specified on the
command-line? Sure.

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.

We don't actually have any "blended" tuning at the moment...cortex-a8
tuning is probably at least as good as anything else we have at the
moment.

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?”

Sounds fine.

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.

I'm not following. sys::cl command-line options aren't relevant to
clang command-line processing.

I'm not sure about 3, 5, and 6; probably Daniel or Eric knows.

-Eli

Hi Eli,

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.

We don't actually have any "blended" tuning at the moment...cortex-a8
tuning is probably at least as good as anything else we have at the
moment.

Sure; what I was thinking was even though we don't have that sort of tuning,
it's not right to force a specific core. OK, for ARM, cortex-a8 will
probably give the best performance all round as it actually has an
Itinerary, but looking forward that's probably not right.

The main thing is that being able to remove this would remove 50+ lines of
horrible crufty switch statements, which I think are not needed.

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.

I'm not following. sys::cl command-line options aren't relevant to
clang command-line processing.

Apologies, my description made a bit of a leap between tools without
explaining. -mno-global-merge is passed down from the driver to llc, where
it is picked up in Target/ARM/ARMTargetMachine.cpp by a sys::cl static. I
should think that sys::cl should be able to deal with having repeated
"-mno-global-merge -mglobal-merge" arguments and picking the last one
itself, instead of the Driver having to do some massaging?

Cheers,

James

Hi Eli,

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.

We don't actually have any "blended" tuning at the moment...cortex-a8
tuning is probably at least as good as anything else we have at the
moment.

Sure; what I was thinking was even though we don't have that sort of tuning,
it's not right to force a specific core. OK, for ARM, cortex-a8 will
probably give the best performance all round as it actually has an
Itinerary, but looking forward that's probably not right.

The main thing is that being able to remove this would remove 50+ lines of
horrible crufty switch statements, which I think are not needed.

Well, we don't really want to change the behavior for Apple targets
here. Given that, if you think you have a better approach, fine.

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.

I'm not following. sys::cl command-line options aren't relevant to
clang command-line processing.

Apologies, my description made a bit of a leap between tools without
explaining. -mno-global-merge is passed down from the driver to llc, where
it is picked up in Target/ARM/ARMTargetMachine.cpp by a sys::cl static. I
should think that sys::cl should be able to deal with having repeated
"-mno-global-merge -mglobal-merge" arguments and picking the last one
itself, instead of the Driver having to do some massaging?

The textual representation of the command-line is completely gone by
the time we invoke CodeGen. Communicating with CodeGen via sys::cl is
less than ideal, but nobody has volunteered to clean it up.

-Eli

Well, we don't really want to change the behavior for Apple targets
here. Given that, if you think you have a better approach, fine.

If (Darwin && v7) Cpu = cortex-a8.

This keeps the current behaviour for Darwin, fits in nicely with the rest of the Darwin special cases, and doesn't need a shedload of switch/cases because the only Cpu with nonstandard behaviour (because it has an Itinerary) is A8. (Excluding A9, which isn't any architectural default).

The textual representation of the command-line is completely gone by
the time we invoke CodeGen. Communicating with CodeGen via sys::cl is
less than ideal, but nobody has volunteered to clean it up.

Right, sounds like it's there for a reason. I'll keep it and add a FIXME.

Cheers,

James