Hi Gabor, Joerg To me, it seems like an easy win to have better GCC compatibility for options like -mcpu which mirror GCC options. In terms of lack of uses in the wild, it appears to be a behaviour change in GCC 4.9 so would only appear in very new projects. The behaviour is not documented, but there is no reason it could not appear in projects in the future.
Other ARM compilers e.g. IAR, armcc, gcc accept capitalised names for their equivalent options and ARM tends to refer to the CPUs via captialised names like Cortex-A72 so it is reasonable to assume someone might try to pass -mcpu=Cortex-A72 to clang or to gcc and expect it to work. Is there a good reason not to follow gcc here? On the patch itself: * Is there any reason not to patch the function DecodeAArch64Mcpu itself like for GetArmArchForMCpu rather than patching the arguments at the two callsites? * Otherwise this looks ok to me - but IANA Clang expert. Thanks Rich > On Mon, May 04, 2015 at 04:30:40PM +0000, Gabor Ballabas wrote: > > GCC allows case-insensitive values for -mcpu, -march and -mtune options. > > Is there really a good reason to follow? I haven't seen such use in the > wild. > > Joerg > -----Original Message----- > From: Gabor Ballabas [mailto:[email protected]] > Sent: 04 May 2015 17:31 > To: [email protected]; Richard Barton; Kristof Beyls > Cc: [email protected]; Amara Emerson; [email protected]; > [email protected]; [email protected]; [email protected] > Subject: [PATCH] [PATCH] Allow case-insensitive values for -mcpu for ARM > and AArch64 targets in line with GCC. > > Hi richard.barton.arm, kristof.beyls, > > GCC allows case-insensitive values for -mcpu, -march and -mtune options. > This patch implements the same behaviour for the -mcpu option. > I also plan to do the same for -march and -mtune, I just wanted to keep them > separate. > > I am not sure about how to test it though. There are hundreds of tests for - > mcpu > in the test/Driver directory. Should I duplicate all of them in the existing > test > files > with upper-case -mcpu arguments, or should I create a separate test file for > them? > Or maybe just have some of them duplicated? > I'm looking forward to some guidance here. > > Thanks, > Gabor Ballabas > > REPOSITORY > rL LLVM > > http://reviews.llvm.org/D9476 > > Files: > lib/Driver/ToolChains.cpp > lib/Driver/Tools.cpp > > Index: lib/Driver/ToolChains.cpp > ========================================================== > ========= > --- lib/Driver/ToolChains.cpp > +++ lib/Driver/ToolChains.cpp > @@ -126,7 +126,7 @@ > } > > static const char *GetArmArchForMCpu(StringRef Value) { > - return llvm::StringSwitch<const char *>(Value) > + return llvm::StringSwitch<const char *>(Value.lower()) > .Cases("arm9e", "arm946e-s", "arm966e-s", "arm968e-s", "arm926ej- > s","armv5") > .Cases("arm10e", "arm10tdmi", "armv5") > .Cases("arm1020t", "arm1020e", "arm1022e", "arm1026ej-s", "armv5") > Index: lib/Driver/Tools.cpp > ========================================================== > ========= > --- lib/Driver/Tools.cpp > +++ lib/Driver/Tools.cpp > @@ -895,7 +895,7 @@ > if ((A = Args.getLastArg(options::OPT_mtune_EQ))) { > CPU = A->getValue(); > } else if ((A = Args.getLastArg(options::OPT_mcpu_EQ))) { > - StringRef Mcpu = A->getValue(); > + StringRef Mcpu = StringRef(A->getValue()).lower(); > CPU = Mcpu.split("+").first; > } > > @@ -1826,7 +1826,7 @@ > const ArgList &Args, > std::vector<const char *> &Features) { > StringRef CPU; > - if (!DecodeAArch64Mcpu(D, Mcpu, CPU, Features)) > + if (!DecodeAArch64Mcpu(D, Mcpu.lower(), CPU, Features)) > return false; > > return true; > @@ -1852,7 +1852,7 @@ > std::vector<const char *> &Features) { > StringRef CPU; > std::vector<const char *> DecodedFeature; > - if (!DecodeAArch64Mcpu(D, Mcpu, CPU, DecodedFeature)) > + if (!DecodeAArch64Mcpu(D, Mcpu.lower(), CPU, DecodedFeature)) > return false; > > return getAArch64MicroArchFeaturesFromMtune(D, CPU, Args, Features); > @@ -5613,7 +5613,7 @@ > // FIXME: Warn on inconsistent use of -mcpu and -march. > // If we have -mcpu=, use that. > if (Arg *A = Args.getLastArg(options::OPT_mcpu_EQ)) { > - StringRef MCPU = A->getValue(); > + StringRef MCPU = StringRef(A->getValue()).lower(); > // Handle -mcpu=native. > if (MCPU == "native") > return llvm::sys::getHostCPUName(); > @@ -7482,7 +7482,7 @@ > // march from being picked in the absence of a cpu flag. > Arg *A; > if ((A = Args.getLastArg(options::OPT_mcpu_EQ)) && > - StringRef(A->getValue()) == "krait") > + StringRef(A->getValue()).lower() == "krait") > CmdArgs.push_back("-march=armv7-a"); > else > Args.AddLastArg(CmdArgs, options::OPT_mcpu_EQ); > > EMAIL PREFERENCES > http://reviews.llvm.org/settings/panel/emailpreferences/ _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
