[AArch64][SVE] Wire up vscale_range attribute to SVE min/max vector queries

[bsmith]

• Fix minor mistake in test

[joechrisellis]

Looks good to me. 😄

Do you think it might also be worth adding a clang test so that we can verify that the -msve-vector-bits=N flag is working end-to-end?

[sdesmalen]

Just left a new nits, but otherwise looks fine to me.

llvm/lib/Target/AArch64/AArch64Subtarget.cpp

214

nit: is it worth making these Optional, if the subtarget doesn't have SVE?

226

nit: Does this need an assert that Min|MaxSVEVectorSizeInBits is zero when SVE is not enabled in the feature string?

llvm/test/CodeGen/AArch64/sve-vscale-attr.ll

134–139

nit: I'm not sure if the attributes need to be at the end of the file necessarily, but if not, can you move each of them below the function that uses it?

[Matt]

llvm/unittests/Target/AArch64/InstSizes.cpp

33

Nit: Adding parameter comments similar to the existing /* isLittle */ false may be a bit clearer than 0, 0.

[paulwalker-arm]

llvm/lib/Target/AArch64/AArch64Subtarget.cpp

220–225

What about moving the aarch64-sve-vector-bits command line opts into AArch64TargetMachine.cpp and resolving the conflict there? That way all the logic is in one place.

369–373

Based on my previous comment I think these two asserts can be move into AArch64TargetMachine.cpp also, and/or perhaps AArch64Subtarget::AArch64Subtarget.

376

Again assuming my "move opts to AArch64TargetMachine.cpp" suggestion is sound I think this "normalise user input" code can sit in AArch64TargetMachine.cpp also.

381–385

Same comment as with getMaxSVEVectorSizeInBits.

386–388

Based on my other comments I'm wondering if we just got to the point where both these functions need to be moved into AArch64TargetMachine.cpp with AArch64Subtarget having simple accessors.

llvm/lib/Target/AArch64/AArch64TargetMachine.cpp

357

I don't know if this is possible but I feel we need a HasSVE like check here?

[bsmith]

llvm/lib/Target/AArch64/AArch64Subtarget.cpp

386–388

I don't think we can go quite this far as AArch64TargetMachine is not function specific like the attributes are. I think we can move all of the logic of resolving between the options and the attribute, but the storage/accessors for the resulting value needs to live in the subtarget.

[bsmith]

• Move attribute/command line logic into AArch64TargetMachine.
• Fix issue with subtarget key appending integers.
• Add end-to-end test.
• Make new subtarget options optional.

[bsmith]

llvm/lib/Target/AArch64/AArch64Subtarget.cpp

226

In principal you could, however I'm not sure it adds any value, as the accessors already assert that SVE is enabled. (And in principal this is a generic attribute, not an SVE one).

llvm/lib/Target/AArch64/AArch64TargetMachine.cpp

357

I'm not sure this is really doable here without picking apart the feature string, I think it makes more sense to just set the values and assert when using the accessors without SVE enabled.

[paulwalker-arm]

llvm/lib/Target/AArch64/AArch64Subtarget.h

298–299

Out of interest are these defaults ever relied upon?

llvm/lib/Target/AArch64/AArch64TargetMachine.cpp

357

Fair enough.

367–373

These asserts are fine but you'll see from the original implementations of getM..SVEVectorSizeInBits that I do not rely on the user passing the correct values. Instead I also always process the sizes to ensure the values of MinSVEVectorSize and MaxSVEVectorSizeare sane. Can you do likewise here?

[bsmith]

• Ensure user input is sanitized for when asserts are not enabled
• Fix clang format issues

llvm/lib/Target/AArch64/AArch64Subtarget.h

298–299

Only in the case of manual construction of a subtarget, for example a unit test etc. For normal cases this will always be passed in based on the attribute or command line options.

[paulwalker-arm]

llvm/lib/Target/AArch64/AArch64Subtarget.cpp

367–376

Up to you but now these are simple accessors, is it worth having the implementations inlined into the header?

[FlameTop]

This commit is failing the test on non-arm targets (e.g. https://lab.llvm.org/buildbot/#/builders/139/builds/5932). I think the test needs a 'requires' line for aarch64.

[bsmith]

Yup, just committed a fix in ed31ff9c7a9e538ead1fa4feecf09987998621b4, sorry for the noise.

[Matt]

llvm/lib/Target/AArch64/AArch64TargetMachine.cpp

161

Just noticed a small typo: s/AAcrh64/AArch64/

llvm/lib/Target/AArch64/AArch64Subtarget.h

protected:
   // ReserveXRegister[i] - X#i is not available as a general purpose register.
   BitVector ReserveXRegister;
 
   // CustomCallUsedXRegister[i] - X#i call saved.
   BitVector CustomCallSavedXRegs;
 
   bool IsLittle;
 
+  unsigned MinSVEVectorSizeInBits;
+  unsigned MaxSVEVectorSizeInBits;
+
   /// TargetTriple - What processor and OS we're targeting.
   Triple TargetTriple;
 
   AArch64FrameLowering FrameLowering;
   AArch64InstrInfo InstrInfo;
   AArch64SelectionDAGInfo TSInfo;
   AArch64TargetLowering TLInfo;
 
private:
   /// Initialize properties based on the selected processor family.
   void initializeProperties();
 
 public:
   /// This constructor initializes the data members to match that
   /// of the specified triple.
   AArch64Subtarget(const Triple &TT, const std::string &CPU,
                    const std::string &FS, const TargetMachine &TM,
-                   bool LittleEndian);
+                   bool LittleEndian, unsigned MinSVEVectorSizeInBitsOverride,
+                   unsigned MaxSVEVectorSizeInBitsOverride);
 
   const AArch64SelectionDAGInfo *getSelectionDAGInfo() const override {
     return &TSInfo;
   }
   const AArch64FrameLowering *getFrameLowering() const override {
     return &FrameLowering;
   }
   const AArch64TargetLowering *getTargetLowering() const override {
     return &TLInfo;

llvm/lib/Target/AArch64/AArch64Subtarget.cpp

 UseAddressTopByteIgnored("aarch64-use-tbi", cl::desc("Assume that top byte of "
                          "an address is ignored"), cl::init(false), cl::Hidden);
 
 static cl::opt<bool>
     UseNonLazyBind("aarch64-enable-nonlazybind",
                    cl::desc("Call nonlazybind functions via direct GOT load"),
                    cl::init(false), cl::Hidden);
 
-static cl::opt<unsigned> SVEVectorBitsMax(
+static cl::opt<unsigned> SVEVectorBitsMaxOpt(
     "aarch64-sve-vector-bits-max",
     cl::desc("Assume SVE vector registers are at most this big, "
              "with zero meaning no maximum size is assumed."),
     cl::init(0), cl::Hidden);
 
-static cl::opt<unsigned> SVEVectorBitsMin(
+static cl::opt<unsigned> SVEVectorBitsMinOpt(
     "aarch64-sve-vector-bits-min",
     cl::desc("Assume SVE vector registers are at least this big, "
              "with zero meaning no minimum size is assumed."),
     cl::init(0), cl::Hidden);
 
 static cl::opt<bool> UseAA("aarch64-use-aa", cl::init(true),
                            cl::desc("Enable the use of AA during codegen."));
 
case ThunderX3T110:
     // FIXME: remove this to enable 64-bit SLP if performance looks good.
     MinVectorRegisterBitWidth = 128;
     break;
   }
 }
 
 AArch64Subtarget::AArch64Subtarget(const Triple &TT, const std::string &CPU,
                                    const std::string &FS,
-                                   const TargetMachine &TM, bool LittleEndian)
+                                   const TargetMachine &TM, bool LittleEndian,
+                                   unsigned MinSVEVectorSizeInBitsOverride,
+                                   unsigned MaxSVEVectorSizeInBitsOverride)
     : AArch64GenSubtargetInfo(TT, CPU, /*TuneCPU*/ CPU, FS),
       ReserveXRegister(AArch64::GPR64commonRegClass.getNumRegs()),
       CustomCallSavedXRegs(AArch64::GPR64commonRegClass.getNumRegs()),
       IsLittle(LittleEndian),
+      MinSVEVectorSizeInBits(MinSVEVectorSizeInBitsOverride
+                                 ? MinSVEVectorSizeInBitsOverride
+                                 : SVEVectorBitsMinOpt),
+      MaxSVEVectorSizeInBits(MaxSVEVectorSizeInBitsOverride
+                                 ? MaxSVEVectorSizeInBitsOverride
+                                 : SVEVectorBitsMaxOpt),
       TargetTriple(TT), FrameLowering(),
       InstrInfo(initializeSubtargetDependencies(FS, CPU)), TSInfo(),
       TLInfo(TM, *this) {
   if (AArch64::isX18ReservedByDefault(TT))
     ReserveXRegister.set(18);
 
   CallLoweringInfo.reset(new AArch64CallLowering(*getTargetLowering()));
   InlineAsmLoweringInfo.reset(new InlineAsmLowering(getTargetLowering()));
   Legalizer.reset(new AArch64LegalizerInfo(*this));
void AArch64Subtarget::mirFileLoaded(MachineFunction &MF) const {
   // if the .mir file didn't specify it. Note that this will probably give you
   // bogus values after PEI has eliminated the callframe setup/destroy pseudo
   // instructions, specify explicitly if you need it to be correct.
   MachineFrameInfo &MFI = MF.getFrameInfo();
   if (!MFI.isMaxCallFrameSizeComputed())
     MFI.computeMaxCallFrameSize(MF);
 }
 
 unsigned AArch64Subtarget::getMaxSVEVectorSizeInBits() const {
   assert(HasSVE && "Tried to get SVE vector length without SVE support!");
-  assert(SVEVectorBitsMax % 128 == 0 &&
+  assert(MaxSVEVectorSizeInBits % 128 == 0 &&
          "SVE requires vector length in multiples of 128!");
-  assert((SVEVectorBitsMax >= SVEVectorBitsMin || SVEVectorBitsMax == 0) &&
+  assert((MaxSVEVectorSizeInBits >= MinSVEVectorSizeInBits ||
+          MaxSVEVectorSizeInBits == 0) &&
          "Minimum SVE vector size should not be larger than its maximum!");
-  if (SVEVectorBitsMax == 0)
+  if (MaxSVEVectorSizeInBits == 0)
     return 0;
-  return (std::max(SVEVectorBitsMin, SVEVectorBitsMax) / 128) * 128;
+  return (std::max(MinSVEVectorSizeInBits, MaxSVEVectorSizeInBits) / 128) * 128;
 }
 
 unsigned AArch64Subtarget::getMinSVEVectorSizeInBits() const {
   assert(HasSVE && "Tried to get SVE vector length without SVE support!");
-  assert(SVEVectorBitsMin % 128 == 0 &&
+  assert(MinSVEVectorSizeInBits % 128 == 0 &&
          "SVE requires vector length in multiples of 128!");
-  assert((SVEVectorBitsMax >= SVEVectorBitsMin || SVEVectorBitsMax == 0) &&
+  assert((MaxSVEVectorSizeInBits >= MinSVEVectorSizeInBits ||
+          MaxSVEVectorSizeInBits == 0) &&
          "Minimum SVE vector size should not be larger than its maximum!");
-  if (SVEVectorBitsMax == 0)
-    return (SVEVectorBitsMin / 128) * 128;
-  return (std::min(SVEVectorBitsMin, SVEVectorBitsMax) / 128) * 128;
+  if (MaxSVEVectorSizeInBits == 0)
+    return (MinSVEVectorSizeInBits / 128) * 128;
+  return (std::min(MinSVEVectorSizeInBits, MaxSVEVectorSizeInBits) / 128) * 128;
 }
 
 bool AArch64Subtarget::useSVEForFixedLengthVectors() const {
   // Prefer NEON unless larger SVE registers are available.
   return hasSVE() && getMinSVEVectorSizeInBits() >= 256;
 }
 
 bool AArch64Subtarget::useAA() const { return UseAA; }

llvm/lib/Target/AArch64/AArch64TargetMachine.cpp

EnableSVEIntrinsicOpts("aarch64-enable-sve-intrinsic-opts", cl::Hidden,
                            cl::desc("Enable SVE intrinsic opts"),
                            cl::init(true));
 
 static cl::opt<bool> EnableFalkorHWPFFix("aarch64-enable-falkor-hwpf-fix",
                                          cl::init(true), cl::Hidden);
 
 static cl::opt<bool>
     EnableBranchTargets("aarch64-enable-branch-targets", cl::Hidden,
                         cl::desc("Enable the AAcrh64 branch target pass"),
                         cl::init(true));
 
 extern cl::opt<bool> EnableHomogeneousPrologEpilog;
 
 extern "C" LLVM_EXTERNAL_VISIBILITY void LLVMInitializeAArch64Target() {
   // Register the target.
   RegisterTargetMachine<AArch64leTargetMachine> X(getTheAArch64leTarget());
   RegisterTargetMachine<AArch64beTargetMachine> Y(getTheAArch64beTarget());
AArch64TargetMachine::getSubtargetImpl(const Function &F) const {
   Attribute CPUAttr = F.getFnAttribute("target-cpu");
   Attribute FSAttr = F.getFnAttribute("target-features");
 
   std::string CPU =
       CPUAttr.isValid() ? CPUAttr.getValueAsString().str() : TargetCPU;
   std::string FS =
       FSAttr.isValid() ? FSAttr.getValueAsString().str() : TargetFS;
 
-  auto &I = SubtargetMap[CPU + FS];
+  SmallString<512> Key;
+
+  unsigned MinSVEVectorSize = 0;
+  unsigned MaxSVEVectorSize = 0;
+  Attribute VScaleRangeAttr = F.getFnAttribute(Attribute::VScaleRange);
+  if (VScaleRangeAttr.isValid())
+    std::tie(MinSVEVectorSize, MaxSVEVectorSize) =
+        VScaleRangeAttr.getVScaleRangeArgs();
+
+  Key += "SVEMin";
+  Key += MinSVEVectorSize;
+  Key += "SVEMax";
+  Key += MaxSVEVectorSize;
+  Key += CPU;
+  Key += FS;
+
+  auto &I = SubtargetMap[Key];
   if (!I) {
     // This needs to be done before we create a new subtarget since any
     // creation will depend on the TM and the code generation flags on the
     // function that reside in TargetOptions.
     resetTargetOptions(F);
     I = std::make_unique<AArch64Subtarget>(TargetTriple, CPU, FS, *this,
-                                            isLittle);
+                                           isLittle, MinSVEVectorSize * 128,
+                                           MaxSVEVectorSize * 128);
   }
   return I.get();
 }
 
 void AArch64leTargetMachine::anchor() { }
 
 AArch64leTargetMachine::AArch64leTargetMachine(
     const Target &T, const Triple &TT, StringRef CPU, StringRef FS,

llvm/test/CodeGen/AArch64/sve-vscale-attr.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc < %s | FileCheck %s --check-prefixes=CHECK,CHECK-NOARG
+; RUN: llc -aarch64-sve-vector-bits-min=512 < %s | FileCheck %s --check-prefixes=CHECK,CHECK-ARG
+
+target triple = "aarch64-unknown-linux-gnu"
+
+define void @func_vscale_none(<16 x i32>* %a, <16 x i32>* %b) #0 {
+; CHECK-NOARG-LABEL: func_vscale_none:
+; CHECK-NOARG:       // %bb.0:
+; CHECK-NOARG-NEXT:    ldp q0, q1, [x0]
+; CHECK-NOARG-NEXT:    ldp q2, q3, [x1]
+; CHECK-NOARG-NEXT:    ldp q4, q5, [x0, #32]
+; CHECK-NOARG-NEXT:    ldp q7, q6, [x1, #32]
+; CHECK-NOARG-NEXT:    add v1.4s, v1.4s, v3.4s
+; CHECK-NOARG-NEXT:    add v0.4s, v0.4s, v2.4s
+; CHECK-NOARG-NEXT:    add v2.4s, v5.4s, v6.4s
+; CHECK-NOARG-NEXT:    add v3.4s, v4.4s, v7.4s
+; CHECK-NOARG-NEXT:    stp q3, q2, [x0, #32]
+; CHECK-NOARG-NEXT:    stp q0, q1, [x0]
+; CHECK-NOARG-NEXT:    ret
+;
+; CHECK-ARG-LABEL: func_vscale_none:
+; CHECK-ARG:       // %bb.0:
+; CHECK-ARG-NEXT:    ptrue p0.s, vl16
+; CHECK-ARG-NEXT:    ld1w { z0.s }, p0/z, [x0]
+; CHECK-ARG-NEXT:    ld1w { z1.s }, p0/z, [x1]
+; CHECK-ARG-NEXT:    add z0.s, p0/m, z0.s, z1.s
+; CHECK-ARG-NEXT:    st1w { z0.s }, p0, [x0]
+; CHECK-ARG-NEXT:    ret
+  %op1 = load <16 x i32>, <16 x i32>* %a
+  %op2 = load <16 x i32>, <16 x i32>* %b
+  %res = add <16 x i32> %op1, %op2
+  store <16 x i32> %res, <16 x i32>* %a
+  ret void
+}
+
+define void @func_vscale1_1(<16 x i32>* %a, <16 x i32>* %b) #1 {
+; CHECK-LABEL: func_vscale1_1:
+; CHECK:       // %bb.0:
+; CHECK-NEXT:    ldp q0, q1, [x0]
+; CHECK-NEXT:    ldp q2, q3, [x1]
+; CHECK-NEXT:    ldp q4, q5, [x0, #32]
+; CHECK-NEXT:    ldp q7, q6, [x1, #32]
+; CHECK-NEXT:    add v1.4s, v1.4s, v3.4s
+; CHECK-NEXT:    add v0.4s, v0.4s, v2.4s
+; CHECK-NEXT:    add v2.4s, v5.4s, v6.4s
+; CHECK-NEXT:    add v3.4s, v4.4s, v7.4s
+; CHECK-NEXT:    stp q3, q2, [x0, #32]
+; CHECK-NEXT:    stp q0, q1, [x0]
+; CHECK-NEXT:    ret
+  %op1 = load <16 x i32>, <16 x i32>* %a
+  %op2 = load <16 x i32>, <16 x i32>* %b
+  %res = add <16 x i32> %op1, %op2
+  store <16 x i32> %res, <16 x i32>* %a
+  ret void
+}
+
+define void @func_vscale2_2(<16 x i32>* %a, <16 x i32>* %b) #2 {
+; CHECK-LABEL: func_vscale2_2:
+; CHECK:       // %bb.0:
+; CHECK-NEXT:    ptrue p0.s, vl8
+; CHECK-NEXT:    add x8, x0, #32 // =32
+; CHECK-NEXT:    add x9, x1, #32 // =32
+; CHECK-NEXT:    ld1w { z0.s }, p0/z, [x0]
+; CHECK-NEXT:    ld1w { z1.s }, p0/z, [x8]
+; CHECK-NEXT:    ld1w { z2.s }, p0/z, [x1]
+; CHECK-NEXT:    ld1w { z3.s }, p0/z, [x9]
+; CHECK-NEXT:    add z0.s, p0/m, z0.s, z2.s
+; CHECK-NEXT:    add z1.s, p0/m, z1.s, z3.s
+; CHECK-NEXT:    st1w { z0.s }, p0, [x0]
+; CHECK-NEXT:    st1w { z1.s }, p0, [x8]
+; CHECK-NEXT:    ret
+  %op1 = load <16 x i32>, <16 x i32>* %a
+  %op2 = load <16 x i32>, <16 x i32>* %b
+  %res = add <16 x i32> %op1, %op2
+  store <16 x i32> %res, <16 x i32>* %a
+  ret void
+}
+
+define void @func_vscale2_4(<16 x i32>* %a, <16 x i32>* %b) #3 {
+; CHECK-LABEL: func_vscale2_4:
+; CHECK:       // %bb.0:
+; CHECK-NEXT:    ptrue p0.s, vl8
+; CHECK-NEXT:    add x8, x0, #32 // =32
+; CHECK-NEXT:    add x9, x1, #32 // =32
+; CHECK-NEXT:    ld1w { z0.s }, p0/z, [x0]
+; CHECK-NEXT:    ld1w { z1.s }, p0/z, [x8]
+; CHECK-NEXT:    ld1w { z2.s }, p0/z, [x1]
+; CHECK-NEXT:    ld1w { z3.s }, p0/z, [x9]
+; CHECK-NEXT:    add z0.s, p0/m, z0.s, z2.s
+; CHECK-NEXT:    add z1.s, p0/m, z1.s, z3.s
+; CHECK-NEXT:    st1w { z0.s }, p0, [x0]
+; CHECK-NEXT:    st1w { z1.s }, p0, [x8]
+; CHECK-NEXT:    ret
+  %op1 = load <16 x i32>, <16 x i32>* %a
+  %op2 = load <16 x i32>, <16 x i32>* %b
+  %res = add <16 x i32> %op1, %op2
+  store <16 x i32> %res, <16 x i32>* %a
+  ret void
+}
+
+define void @func_vscale4_4(<16 x i32>* %a, <16 x i32>* %b) #4 {
+; CHECK-LABEL: func_vscale4_4:
+; CHECK:       // %bb.0:
+; CHECK-NEXT:    ptrue p0.s, vl16
+; CHECK-NEXT:    ld1w { z0.s }, p0/z, [x0]
+; CHECK-NEXT:    ld1w { z1.s }, p0/z, [x1]
+; CHECK-NEXT:    add z0.s, p0/m, z0.s, z1.s
+; CHECK-NEXT:    st1w { z0.s }, p0, [x0]
+; CHECK-NEXT:    ret
+  %op1 = load <16 x i32>, <16 x i32>* %a
+  %op2 = load <16 x i32>, <16 x i32>* %b
+  %res = add <16 x i32> %op1, %op2
+  store <16 x i32> %res, <16 x i32>* %a
+  ret void
+}
+
+define void @func_vscale8_8(<16 x i32>* %a, <16 x i32>* %b) #5 {
+; CHECK-LABEL: func_vscale8_8:
+; CHECK:       // %bb.0:
+; CHECK-NEXT:    ptrue p0.s, vl16
+; CHECK-NEXT:    ld1w { z0.s }, p0/z, [x0]
+; CHECK-NEXT:    ld1w { z1.s }, p0/z, [x1]
+; CHECK-NEXT:    add z0.s, p0/m, z0.s, z1.s
+; CHECK-NEXT:    st1w { z0.s }, p0, [x0]
+; CHECK-NEXT:    ret
+  %op1 = load <16 x i32>, <16 x i32>* %a
+  %op2 = load <16 x i32>, <16 x i32>* %b
+  %res = add <16 x i32> %op1, %op2
+  store <16 x i32> %res, <16 x i32>* %a
+  ret void
+}
+
+attributes #0 = { "target-features"="+sve" }
+attributes #1 = { "target-features"="+sve" vscale_range(1,1) }
+attributes #2 = { "target-features"="+sve" vscale_range(2,2) }
+attributes #3 = { "target-features"="+sve" vscale_range(2,4) }
+attributes #4 = { "target-features"="+sve" vscale_range(4,4) }
+attributes #5 = { "target-features"="+sve" vscale_range(8,8) }

llvm/unittests/Target/AArch64/InstSizes.cpp

std::unique_ptr<LLVMTargetMachine> createTargetMachine() {
   return std::unique_ptr<LLVMTargetMachine>(static_cast<LLVMTargetMachine*>(
       TheTarget->createTargetMachine(TT, CPU, FS, TargetOptions(), None, None,
                                      CodeGenOpt::Default)));
 }
 
 std::unique_ptr<AArch64InstrInfo> createInstrInfo(TargetMachine *TM) {
   AArch64Subtarget ST(TM->getTargetTriple(), std::string(TM->getTargetCPU()),
                       std::string(TM->getTargetFeatureString()), *TM,
-                      /* isLittle */ false);
+                      /* isLittle */ false, 0, 0);
   return std::make_unique<AArch64InstrInfo>(ST);
 }
 
 /// The \p InputIRSnippet is only needed for things that can't be expressed in
 /// the \p InputMIRSnippet (global variables etc)
 /// TODO: Some of this might be useful for other architectures as well - extract
 ///       the platform-independent parts somewhere they can be reused.
 void runChecks(