[ARM] Do not test for CPUs, use SubtargetFeatures (Part 1). NFC

[rovka]

Summary

This is a cleanup commit similar to r271555, but for ARM.

The end goal is to get rid of the isSwift / isCortexXY / isWhatever methods.

Since the ARM backend seems to have quite a lot of calls to these methods, I intend to submit 5-6 subtarget features at a time, instead of one big lump.

[rengolin]

lib/Target/ARM/ARMInstrInfo.td

327

probably don't need the comments any more?

[rovka]

Remove redundant comments.

[MatzeB]

Looks mostly good to me, but:

lib/Target/ARM/ARMSubtarget.h

221–237

This needs to be initialized, you may want to do a similar commit as I did in r271057 and use member initializers throughout the Subtarget class.

[compnerd]

lib/Target/ARM/ARM.td

111

I think that it may be easier to actually have this be inverted and have a "cheap predicates" feature which would normally be false.

127

Why not spell out prefer? I don't think that the two extra characters are that bad here.

132

Similar.

137

FeatureNEONFPMoves perhaps?

lib/Target/ARM/ARMInstrInfo.td

341

Wrap these two lines? They seem beyond 80 characters.

[rovka]

Addressed review comments.

[rovka]

Thanks for having a look.

lib/Target/ARM/ARM.td

111

This takes its name from TargetInstrInfo::IsProfitableToUnpredicate, which is where it's used. I'm not sure it's worth the trouble of renaming that for all the targets.

132

Did some spelling out, but I'd rather not spell it out in the feature name, as that would make it go over 80 chars in the features list (and I'd rather keep feature names short than realign the whole thing). There's a precedent for this in FeaturePref32BitThumb.

137

There's already a FeatureNEONForFP, I followed that one (although I'm not against renaming this or both of them).

While we're at it: I find it a bit peculiar that FeatureNEONForFP is enabled only for Swift and Cyclone, and FeatureNEONForFPMoves is enabled only for Cortex-A9. I'm not sure if this is an oversight or if I just picked a really bad name for this feature. Any thoughts?

lib/Target/ARM/ARMSubtarget.h

221–237

Oops... Looks like for ARM this is done in ARMSubtarget::initializeEnvironment, so I added it there.

[MatzeB]

lib/Target/ARM/ARMSubtarget.h

221–237

I find member initializers a lot less error prone than having the initializers in a separate method. But we can fix that in another commit.

[sbaranga]

lib/Target/ARM/ARM.td

137

They are both core-specific optimizations, so it makes sense that they are enabled selectively:

FeatureNEONForFPMoves enables changing the execution domain for mov-like instructions from VFP to NEON and is a core-specific optimization, so it makes sense to me that it's only enabled on Cortex-A9 (unless we have additional data points).

FeatureNEONforFP forces almost all floating point operations to be performed with NEON instructions (which is a problem if we care about how denormals are handled), so it makes sense to me that it's only enabled for swift/cyclone.

I think the distinction is that FeatureNEONForFP uses NEON for VFP movs only when it minimizes the domain crossing between NEON and VFP, while FeatureNEONforFP does this everywhere (for movs and everything else). I'm not sure how you could improve the naming though.

lib/Target/ARM/ARMInstrInfo.td

340

Is there any reason not to test for useNEONForFPMovs instead?

[rengolin]

lib/Target/ARM/ARM.td

137

So, just because one is a sub-set of the other, makes sense to have the same name + radical. But the extra behaviour (NEON/VFP barrier profitability) could be documented, at least in its comment.

About Swift's behaviour to ignore NEON subnormal problem, we have the same issue at the IR level (loop vectorizer), and I have "solved" the old way "ST->isTargetDarwin()", which is true at the platform level.

If you're still not happy with the naming, maybe, such a property could be called "FeatureIgnoreDenormalFP"?

[sbaranga]

lib/Target/ARM/ARM.td

137

Above it should have been "I think the distinction is that FeatureNEONForFPMoves uses NEON for VFP movs only when it minimizes the domain crossing between NEON and VFP, while FeatureNEONforFP does this everywhere (for movs and everything else). I'm not sure how you could improve the naming though.". Sorry if that was a little bit confusing.

I'm not too picky about the naming for either features, they could stay as they are (maybe with some added documentation). Just wanted to point out that the they way these heuristics are used is not an oversight and to give some context in case anyone could come up with a better name for these.

I'm happy with FeatureNEONforFP at the moment, I don't think there's any need to change it.

FWIW I think FeatureNEONforFP is not a superset of FeatureNEONForFPMoves. From a behavioural point of view, there's the heuristic difference. The implementations are also very different (the first being at ISel level, the second is done as part of post-RA optimization). But that's just my interpretation.

[rengolin]

lib/Target/ARM/ARM.td

137

I'm happy with FeatureNEONforFP at the moment, I don't think there's any need to change it.

Ok, me too.

FWIW I think FeatureNEONforFP is not a superset of FeatureNEONForFPMoves. From a behavioural point of view, there's the heuristic difference. The implementations are also very different (the first being at ISel level, the second is done as part of post-RA optimization).

Well, it's about the meaning, not the behaviour. I think it's ok to have behaviour described on comments and meaning encoded in names.

[rovka]

lib/Target/ARM/ARM.td

137

Ok, thanks. I'll try to add more comments.

lib/Target/ARM/ARMInstrInfo.td

340

Not really, just that this seems to be used at ISel level, whereas useNEONForFPMovs is used for execution domain optimization, so I wasn't sure they were in the same boat. I can change it to useNEONForFPMovs if you think that makes sense.

lib/Target/ARM/ARMSubtarget.h

221–237

Ok, I'll try to look into that after I'm done with the subtarget features.

[MatzeB]

In any way LGTM from my side (but maybe wait for Renatos LGTM as well).

[rengolin]

I'm happy with the patch, too. LGTM. Thanks!

lib/Target/ARM/ARMSubtarget.h

221–237

Would be good to create a bugzilla about this, so we don't forget.

[MatzeB]

lib/Target/ARM/ARMSubtarget.h

221–237

IMO switching to member initializers is obvious and trivial enough that it can just be done in an extra commit without review.

[rovka]

Thanks to everyone for the review.

lib/Target/ARM/ARMSubtarget.h

221–237

Ok, I'm going to commit this now and then hopefully get the member initializers in later today.

lib/Target/ARM/ARM.td

def FeatureRAS : SubtargetFeature<"ras", "HasRAS", "true",
                 "Enable Reliability, Availability and Serviceability extensions">;
 
 
 // Cyclone has preferred instructions for zeroing VFP registers, which can
 // execute in 0 cycles.
 def FeatureZCZeroing : SubtargetFeature<"zcz", "HasZeroCycleZeroing", "true",
                                         "Has zero-cycle zeroing instructions">;
 
+// Whether or not it may be profitable to unpredicate certain instructions
+// during if conversion.
+def FeatureProfUnpredicate : SubtargetFeature<"prof-unpr",
+                                              "IsProfitableToUnpredicate",
+                                              "true",
+                                              "Is profitable to unpredicate">;
+
+// Some targets (e.g. Swift) have microcoded VGETLNi32.
+def FeatureSlowVGETLNi32 : SubtargetFeature<"slow-vgetlni32",
+                                            "HasSlowVGETLNi32", "true",
+                                            "Has slow VGETLNi32 - prefer VMOV">;
+
+// Some targets (e.g. Swift) have microcoded VDUP32.
+def FeatureSlowVDUP32 : SubtargetFeature<"slow-vdup32", "HasSlowVDUP32", "true",
+                                         "Has slow VDUP32 - prefer VMOV">;
+
+// Some targets (e.g. Cortex-A9) prefer VMOVSR to VMOVDRR even when using NEON
+// for scalar FP, as this allows more effective execution domain optimization.
+def FeaturePreferVMOVSR : SubtargetFeature<"pref-vmovsr", "PreferVMOVSR",
+                                           "true", "Prefer VMOVSR">;
+
+// Swift has ISHST barriers compatible with Atomic Release semantics but weaker
+// than ISH
+def FeaturePrefISHSTBarrier : SubtargetFeature<"pref-ishst", "PreferISHST",
+                                           "true", "Prefer ISHST barriers">;
+
+// Some targets (e.g. Cortex-A9) want to convert VMOVRS, VMOVSR and VMOVS from
+// VFP to NEON.
+def FeatureNEONForFPMovs : SubtargetFeature<"neon-fpmovs", "UseNEONForFPMovs",
+                              "true", "Convert VMOVSR, VMOVRS, VMOVS to NEON">;
+
 // Some processors have FP multiply-accumulate instructions that don't
 // play nicely with other VFP / NEON instructions, and it's generally better
 // to just not use them.
 def FeatureHasSlowFPVMLx : SubtargetFeature<"slowfpvmlx", "SlowFPVMLx", "true",
                                          "Disable VFP / NEON MAC instructions">;
 
 // Cortex-A8 / A9 Advanced SIMD has multiplier accumulator forwarding.
 def FeatureVMLxForwarding : SubtargetFeature<"vmlx-forwarding",

 
 def : ProcessorModel<"cortex-a9",   CortexA9Model,      [ARMv7a, ProcA9,
                                                          FeatureHasRetAddrStack,
                                                          FeatureTrustZone,
                                                          FeatureVMLxForwarding,
                                                          FeatureT2XtPk,
                                                          FeatureFP16,
                                                          FeatureAvoidPartialCPSR,
+                                                         FeaturePreferVMOVSR,
+                                                         FeatureNEONForFPMovs,
                                                          FeatureMP]>;
 
 // FIXME: A12 has currently the same Schedule model as A9
 def : ProcessorModel<"cortex-a12",  CortexA9Model,      [ARMv7a, ProcA12,
                                                          FeatureHasRetAddrStack,
                                                          FeatureTrustZone,
                                                          FeatureVMLxForwarding,
                                                          FeatureT2XtPk,
def : ProcessorModel<"swift",       SwiftModel,         [ARMv7a, ProcSwift,
                                                          FeatureNEONForFP,
                                                          FeatureT2XtPk,
                                                          FeatureVFP4,
                                                          FeatureMP,
                                                          FeatureHWDiv,
                                                          FeatureHWDivARM,
                                                          FeatureAvoidPartialCPSR,
                                                          FeatureAvoidMOVsShOp,
-                                                         FeatureHasSlowFPVMLx]>;
+                                                         FeatureHasSlowFPVMLx,
+                                                         FeatureProfUnpredicate,
+                                                         FeaturePrefISHSTBarrier,
+                                                         FeatureSlowVGETLNi32,
+                                                         FeatureSlowVDUP32]>;
 
 // FIXME: R4 has currently the same ProcessorModel as A8.
 def : ProcessorModel<"cortex-r4",   CortexA8Model,      [ARMv7r, ProcR4,
                                                          FeatureHasRetAddrStack,
                                                          FeatureAvoidPartialCPSR,
                                                          FeatureT2XtPk]>;
 
 // FIXME: R4F has currently the same ProcessorModel as A8.

lib/Target/ARM/ARMBaseInstrInfo.cpp

   UnpredCost += Subtarget.getMispredictionPenalty() * ScalingUpFactor / 10;
 
   return (TCycles + FCycles + TExtra + FExtra) * ScalingUpFactor <= UnpredCost;
 }
 
 bool
 ARMBaseInstrInfo::isProfitableToUnpredicate(MachineBasicBlock &TMBB,
                                             MachineBasicBlock &FMBB) const {
-  // Reduce false anti-dependencies to let Swift's out-of-order execution
+  // Reduce false anti-dependencies to let the target's out-of-order execution
   // engine do its thing.
-  return Subtarget.isSwift();
+  return Subtarget.isProfitableToUnpredicate();
 }
 
 /// getInstrPredicate - If instruction is predicated, returns its predicate
 /// condition, otherwise returns AL. It also returns the condition code
 /// register by reference.
 ARMCC::CondCodes llvm::getInstrPredicate(const MachineInstr &MI,
                                          unsigned &PredReg) {
   int PIdx = MI.findFirstPredOperandIdx();

   if (Subtarget.hasNEON()) {
     // VMOVD, VMOVRS and VMOVSR are VFP instructions, but can be changed to NEON
     // if they are not predicated.
     if (MI->getOpcode() == ARM::VMOVD && !isPredicated(*MI))
       return std::make_pair(ExeVFP, (1 << ExeVFP) | (1 << ExeNEON));
 
     // CortexA9 is particularly picky about mixing the two and wants these
     // converted.
-    if (Subtarget.isCortexA9() && !isPredicated(*MI) &&
+    if (Subtarget.useNEONForFPMovs() && !isPredicated(*MI) &&
         (MI->getOpcode() == ARM::VMOVRS || MI->getOpcode() == ARM::VMOVSR ||
          MI->getOpcode() == ARM::VMOVS))
       return std::make_pair(ExeVFP, (1 << ExeVFP) | (1 << ExeNEON));
   }
   // No other instructions can be swizzled, so just determine their domain.
   unsigned Domain = MI->getDesc().TSFlags & ARMII::DomainMask;
 
   if (Domain & ARMII::DomainNEON)

lib/Target/ARM/ARMISelLowering.cpp

   }
 
   ConstantSDNode *OrdN = cast<ConstantSDNode>(Op.getOperand(1));
   AtomicOrdering Ord = static_cast<AtomicOrdering>(OrdN->getZExtValue());
   ARM_MB::MemBOpt Domain = ARM_MB::ISH;
   if (Subtarget->isMClass()) {
     // Only a full system barrier exists in the M-class architectures.
     Domain = ARM_MB::SY;
-  } else if (Subtarget->isSwift() && Ord == AtomicOrdering::Release) {
+  } else if (Subtarget->preferISHSTBarriers() &&
+             Ord == AtomicOrdering::Release) {
     // Swift happens to implement ISHST barriers in a way that's compatible with
     // Release semantics but weaker than ISH so we'd be fools not to use
     // it. Beware: other processors probably don't!
     Domain = ARM_MB::ISHST;
   }
 
   return DAG.getNode(ISD::INTRINSIC_VOID, dl, MVT::Other, Op.getOperand(0),
                      DAG.getConstant(Intrinsic::arm_dmb, dl, MVT::i32),

   case AtomicOrdering::Acquire:
     return nullptr; // Nothing to do
   case AtomicOrdering::SequentiallyConsistent:
     if (!IsStore)
       return nullptr; // Nothing to do
     /*FALLTHROUGH*/
   case AtomicOrdering::Release:
   case AtomicOrdering::AcquireRelease:
-    if (Subtarget->isSwift())
+    if (Subtarget->preferISHSTBarriers())
       return makeDMB(Builder, ARM_MB::ISHST);
     // FIXME: add a comment with a link to documentation justifying this.
     else
       return makeDMB(Builder, ARM_MB::ISH);
   }
   llvm_unreachable("Unknown fence ordering in emitLeadingFence");
 }
 

lib/Target/ARM/ARMInstrInfo.td

def UseFusedMAC      : Predicate<"(TM.Options.AllowFPOpFusion =="
                                  " Subtarget->hasVFP4()) && "
                                  "!Subtarget->isTargetDarwin()">;
 def DontUseFusedMAC  : Predicate<"!(TM.Options.AllowFPOpFusion =="
                                  " FPOpFusion::Fast &&"
                                  " Subtarget->hasVFP4()) || "
                                  "Subtarget->isTargetDarwin()">;
 
 // VGETLNi32 is microcoded on Swift - prefer VMOV.
-def HasFastVGETLNi32 : Predicate<"!Subtarget->isSwift()">;
-def HasSlowVGETLNi32 : Predicate<"Subtarget->isSwift()">;
+def HasFastVGETLNi32 : Predicate<"!Subtarget->hasSlowVGETLNi32()">;
+def HasSlowVGETLNi32 : Predicate<"Subtarget->hasSlowVGETLNi32()">;
 
 // VDUP.32 is microcoded on Swift - prefer VMOV.
-def HasFastVDUP32 : Predicate<"!Subtarget->isSwift()">;
-def HasSlowVDUP32 : Predicate<"Subtarget->isSwift()">;
+def HasFastVDUP32 : Predicate<"!Subtarget->hasSlowVDUP32()">;
+def HasSlowVDUP32 : Predicate<"Subtarget->hasSlowVDUP32()">;
 
 // Cortex-A9 prefers VMOVSR to VMOVDRR even when using NEON for scalar FP, as
 // this allows more effective execution domain optimization. See
 // setExecutionDomain().
-def UseVMOVSR : Predicate<"Subtarget->isCortexA9() || !Subtarget->useNEONForSinglePrecisionFP()">;
-def DontUseVMOVSR : Predicate<"!Subtarget->isCortexA9() && Subtarget->useNEONForSinglePrecisionFP()">;
+def UseVMOVSR : Predicate<"Subtarget->preferVMOVSR() || !Subtarget->useNEONForSinglePrecisionFP()">;
+def DontUseVMOVSR : Predicate<"!Subtarget->preferVMOVSR() && Subtarget->useNEONForSinglePrecisionFP()">;
 
 def IsLE             : Predicate<"MF->getDataLayout().isLittleEndian()">;
 def IsBE             : Predicate<"MF->getDataLayout().isBigEndian()">;
 
 //===----------------------------------------------------------------------===//
 // ARM Flag Definitions.
 
 class RegConstraint<string C> {
   string Constraints = C;
 }
 
 //===----------------------------------------------------------------------===//
 //  ARM specific transformation functions and pattern fragments.
 //

lib/Target/ARM/ARMSubtarget.h

protected:
 
   /// HasRAS - if true, the processor supports RAS extensions
   bool HasRAS;
 
   /// If true, the instructions "vmov.i32 d0, #0" and "vmov.i32 q0, #0" are
   /// particularly effective at zeroing a VFP register.
   bool HasZeroCycleZeroing;
 
+  /// If true, if conversion may decide to leave some instructions unpredicated.
+  bool IsProfitableToUnpredicate;
+
+  /// If true, VMOV will be favored over VGETLNi32.
+  bool HasSlowVGETLNi32;
+
+  /// If true, VMOV will be favored over VDUP.
+  bool HasSlowVDUP32;
+
+  /// If true, VMOVSR will be favored over VMOVDRR.
+  bool PreferVMOVSR;
+
+  /// If true, ISHST barriers will be used for Release semantics.
+  bool PreferISHST;
+
+  /// If true, VMOVRS, VMOVSR and VMOVS will be converted from VFP to NEON.
+  bool UseNEONForFPMovs;
+
   /// StrictAlign - If true, the subtarget disallows unaligned memory
   /// accesses for some types.  For details, see
   /// ARMTargetLowering::allowsMisalignedMemoryAccesses().
   bool StrictAlign;
 
   /// RestrictIT - If true, the subtarget disallows generation of deprecated IT
   ///  blocks to conform to ARMv8 rule.
   bool RestrictIT;
public:
   bool useFPVMLx() const { return !SlowFPVMLx; }
   bool hasVMLxForwarding() const { return HasVMLxForwarding; }
   bool isFPBrccSlow() const { return SlowFPBrcc; }
   bool isFPOnlySP() const { return FPOnlySP; }
   bool hasPerfMon() const { return HasPerfMon; }
   bool hasTrustZone() const { return HasTrustZone; }
   bool has8MSecExt() const { return Has8MSecExt; }
   bool hasZeroCycleZeroing() const { return HasZeroCycleZeroing; }
+  bool isProfitableToUnpredicate() const { return IsProfitableToUnpredicate; }
+  bool hasSlowVGETLNi32() const { return HasSlowVGETLNi32; }
+  bool hasSlowVDUP32() const { return HasSlowVDUP32; }
+  bool preferVMOVSR() const { return PreferVMOVSR; }
+  bool preferISHSTBarriers() const { return PreferISHST; }
+  bool useNEONForFPMovs() const { return UseNEONForFPMovs; }
   bool prefers32BitThumb() const { return Pref32BitThumb; }
   bool avoidCPSRPartialUpdate() const { return AvoidCPSRPartialUpdate; }
   bool avoidMOVsShifterOperand() const { return AvoidMOVsShifterOperand; }
   bool hasRetAddrStack() const { return HasRetAddrStack; }
   bool hasMPExtension() const { return HasMPExtension; }
   bool hasDSP() const { return HasDSP; }
   bool useNaClTrap() const { return UseNaClTrap; }
   bool useSjLjEH() const { return UseSjLjEH; }