[cfe-dev] Cleaning up ARM parts of the Clang driver

[James Molloy]

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

-----Original Message-----
From: Eli Friedman [mailto:eli.friedman at gmail.com] 
Sent: 04 October 2011 16:54
To: James Molloy
Cc: cfe-dev at cs.uiuc.edu
Subject: Re: [cfe-dev] Cleaning up ARM parts of the Clang driver

On Tue, Oct 4, 2011 at 4:09 AM, James Molloy <james.molloy at arm.com> wrote:
> 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