[llvm] r206211 - [ARM64][MC] Set the default CPU to cyclone when initilizating the MC layer.
Andrew Trick
atrick at apple.com
Wed Apr 16 12:08:34 PDT 2014
On Apr 16, 2014, at 11:35 AM, Quentin Colombet <qcolombet at apple.com> wrote:
> I don’t have a strong opinion on that.
> This matches what x86 does and to some extend what ARM does for non darwin target.
>
> What would you suggest?
> Emit an error?
Emitting an error and refusing to do something stupid would be fantastic! I guarantee it will save developer time.
> Should we rationalize that for all targets?
Of course a target should be able to provide a cpu type called “generic" if it wants to. If the driver for some platform decides to use “generic” as the default, that’s fine with me. For ARM64, “generic" doesn’t seem very useful except to test certain features of the backend, and certainly should not be a default.
The backend should never assume that “generic” is the default cpu type in a target-independent way. “I forgot to specify a cpu type” is not the same thing as “intentionally generate horrible code so it can run on 20-year-old hardware”.
People should not confuse a “generic” target with a “blended” target that is designed to run passably well across a couple generations of hardware.
I don’t care as much about changing other targets (x86). I realize there are probably drivers out assuming they don’t need to pick a CPU. But why would we back ourselves into the same corner with a new architecture?
-Andy
Not to confuse the issue, but as a side note, the backend could provide logic to pick a default based on the host as long as the driver queries the backend to get the concrete cpu name and explicitly selects the CPU whenever it invokes the backend. The important thing here is generic != default.
>
> Thanks,
> -Quentin
>
> 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.
>>
>> -Andy
>>
>>>
>>> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140416/1222ce22/attachment.html>
More information about the llvm-commits
mailing list