[llvm] r206211 - [ARM64][MC] Set the default CPU to cyclone when initilizating the MC layer.

Wed Apr 16 11:17:14 PDT 2014

On Apr 15, 2014, at 6:25 PM, Andrew Trick <atrick at apple.com> wrote:

> 
> On Apr 14, 2014, at 5:37 PM, Quentin Colombet <qcolombet at apple.com> wrote:
> 
>> I’ve changed the default to generic in r206228.
> 
> FWIW, I don’t think this makes any sense. I don’t think we should generate “generic” arm64 code unless someone explicitly requests it. Otherwise, we’re just hiding a user error or driver bug.

We have two choices, really. Either have a generic default (in this case the literal “generic”) or issue a hard error. Ideally, I generally agree with you. I’d like to have the backends themselves do the latter and have the client layers (EngineBuilder, LLC, clang, et. al.) be entirely responsible for deciding what to do for a default. To do that cleanly would require all backends to have a “generic” CPU name that we could teach those layers to use (and would actually map to whatever the targets themselves want it to), and that’s not the case today. We could certainly add that and make it a requirement for any new backends, but that’d be a moderately disruptive change to out of tree targets and JIT clients. I’m not sure it’s worth it. If someone wants to push forwards with making that happen, though, I wouldn’t be opposed.

-Jim

> 
>> 
>> Thanks,
>> -Quentin
>> 
>> On Apr 14, 2014, at 3:16 PM, Eric Christopher <echristo at gmail.com> wrote:
>> 
>>> Let's go with the "generic" x86 model. It looks like ARM64 does have a
>>> definition for that.
>>> 
>>> -eric
>>> 
>>> On Mon, Apr 14, 2014 at 3:13 PM, Quentin Colombet <qcolombet at apple.com> wrote:
>>>> Well, in fact, I am not sure what is the best approach here.
>>>> 
>>>> ARM and X86 already seem to duplicate this logic somehow, see below.
>>>> 
>>>> So if we are about to duplicate something I’d prefer to keep it minimal like
>>>> in the current patch.
>>>> 
>>>> What do you think?
>>>> 
>>>> ** X86 **
>>>> 
>>>> — X86Subtarget.cpp:
>>>> void X86Subtarget::resetSubtargetFeatures(StringRef CPU, StringRef FS) {
>>>>  std::string CPUName = CPU;
>>>>  if (CPUName.empty())
>>>>    CPUName = "generic";
>>>> 
>>>> — X86MCTargetDesc.cpp:
>>>> MCSubtargetInfo *X86_MC::createX86MCSubtargetInfo(StringRef TT, StringRef
>>>> CPU,
>>>>                                                  StringRef FS) {
>>>> […]
>>>>  std::string CPUName = CPU;
>>>>  if (CPUName.empty()) {
>>>> #if defined(i386) || defined(__i386__) || defined(__x86__) ||
>>>> defined(_M_IX86)\
>>>>    || defined(__x86_64__) || defined(_M_AMD64) || defined (_M_X64)
>>>>    CPUName = sys::getHostCPUName();
>>>> #else
>>>>    CPUName = "generic";
>>>> #endif
>>>>  }
>>>> 
>>>> 
>>>> ** ARM **
>>>> 
>>>> — ARMSubtarget.cpp:
>>>> void ARMSubtarget::resetSubtargetFeatures(StringRef CPU, StringRef FS) {
>>>>  if (CPUString.empty()) {
>>>>    if (isTargetIOS() && TargetTriple.getArchName().endswith("v7s"))
>>>>      // Default to the Swift CPU when targeting armv7s/thumbv7s.
>>>>      CPUString = "swift";
>>>>    else
>>>>      CPUString = "generic";
>>>>  }
>>>> 
>>>> — ARMMCTargetDesc.cpp:
>>>> std::string ARM_MC::ParseARMTriple(StringRef TT, StringRef CPU) {
>>>> [...]
>>>>  bool NoCPU = CPU == "generic" || CPU.empty();
>>>>  std::string ARMArchFeature;
>>>>  if (Idx) {
>>>>    unsigned SubVer = TT[Idx];
>>>>    if (SubVer == '8') {
>>>>      if (NoCPU)
>>>>        // v8a: FeatureDB, FeatureFPARMv8, FeatureNEON, FeatureDSPThumb2,
>>>>        //      FeatureMP, FeatureHWDiv, FeatureHWDivARM, FeatureTrustZone,
>>>>        //      FeatureT2XtPk, FeatureCrypto, FeatureCRC
>>>>        ARMArchFeature =
>>>> "+v8,+db,+fp-armv8,+neon,+t2dsp,+mp,+hwdiv,+hwdiv-arm,"
>>>>                         "+trustzone,+t2xtpk,+crypto,+crc";
>>>>      else
>>>>        // Use CPU to figure out the exact features
>>>>        ARMArchFeature = "+v8”;
>>>> […]
>>>> 
>>>> -Quentin
>>>> 
>>>> On Apr 14, 2014, at 3:02 PM, Eric Christopher <echristo at gmail.com> wrote:
>>>> 
>>>> Thanks!
>>>> 
>>>> I'd probably see the ARM one as closer... it doesn't cope well if
>>>> there's no cpu given it doesn't look like :)
>>>> 
>>>> -eric
>>>> 
>>>> On Mon, Apr 14, 2014 at 3:00 PM, Quentin Colombet <qcolombet at apple.com>
>>>> wrote:
>>>> 
>>>> Sorry, I thought I was doing the same thing as X86 here.
>>>> I’ve been misled by this sequence:
>>>> MCSubtargetInfo *X86_MC::createX86MCSubtargetInfo(StringRef TT, StringRef
>>>> CPU,
>>>>                                                 StringRef FS) {
>>>> [...]
>>>> std::string CPUName = CPU;
>>>> if (CPUName.empty()) {
>>>> #if defined(i386) || defined(__i386__) || defined(__x86__) ||
>>>> defined(_M_IX86)\
>>>>   || defined(__x86_64__) || defined(_M_AMD64) || defined (_M_X64)
>>>>   CPUName = sys::getHostCPUName();
>>>> #else
>>>>   CPUName = "generic";
>>>> #endif
>>>> }
>>>> 
>>>> Let me rework my copy then!
>>>> 
>>>> -Quentin
>>>> 
>>>> On Apr 14, 2014, at 2:42 PM, Eric Christopher <echristo at gmail.com> wrote:
>>>> 
>>>> Uh, what?
>>>> 
>>>> How about taking the same code from the ARM or X86 backends? Or something
>>>> else?
>>>> 
>>>> -eric
>>>> 
>>>> On Mon, Apr 14, 2014 at 2:25 PM, Quentin Colombet <qcolombet at apple.com>
>>>> wrote:
>>>> 
>>>> Author: qcolombet
>>>> Date: Mon Apr 14 16:25:53 2014
>>>> New Revision: 206211
>>>> 
>>>> URL: http://llvm.org/viewvc/llvm-project?rev=206211&view=rev
>>>> Log:
>>>> [ARM64][MC] Set the default CPU to cyclone when initilizating the MC layer.
>>>> This matches that ARM64Subtarget does for now.
>>>> 
>>>> This is related to <rdar://problem/16573920>
>>>> 
>>>> Modified:
>>>>  llvm/trunk/lib/Target/ARM64/MCTargetDesc/ARM64MCTargetDesc.cpp
>>>> 
>>>> Modified: llvm/trunk/lib/Target/ARM64/MCTargetDesc/ARM64MCTargetDesc.cpp
>>>> URL:
>>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/ARM64/MCTargetDesc/ARM64MCTargetDesc.cpp?rev=206211&r1=206210&r2=206211&view=diff
>>>> ==============================================================================
>>>> --- llvm/trunk/lib/Target/ARM64/MCTargetDesc/ARM64MCTargetDesc.cpp
>>>> (original)
>>>> +++ llvm/trunk/lib/Target/ARM64/MCTargetDesc/ARM64MCTargetDesc.cpp Mon Apr
>>>> 14 16:25:53 2014
>>>> @@ -43,6 +43,12 @@ static MCInstrInfo *createARM64MCInstrIn
>>>> static MCSubtargetInfo *createARM64MCSubtargetInfo(StringRef TT, StringRef
>>>> CPU,
>>>>                                                  StringRef FS) {
>>>> MCSubtargetInfo *X = new MCSubtargetInfo();
>>>> +
>>>> +  // FIXME: Make this darwin-only.
>>>> +  if (CPU.empty())
>>>> +    // We default to Cyclone for now, on Darwin.
>>>> +    CPU = "cyclone";
>>>> +
>>>> InitARM64MCSubtargetInfo(X, TT, CPU, FS);
>>>> return X;
>>>> }
>>>> 
>>>> 
>>>> _______________________________________________
>>>> llvm-commits mailing list
>>>> llvm-commits at cs.uiuc.edu
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>> 
>>>> 
>>>> 
>> 
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140416/c5249eed/attachment.html>