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

[Paul Walker via Phabricator via cfe-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64Subtarget.h:298-299
+                   bool LittleEndian,
+                   unsigned MinSVEVectorSizeInBitsOverride = 0,
+                   unsigned MaxSVEVectorSizeInBitsOverride = 0);
 
----------------
Out of interest are these defaults ever relied upon?


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetMachine.cpp:380-386
+  assert(MinSVEVectorSize % 128 == 0 &&
+         "SVE requires vector length in multiples of 128!");
+  assert(MaxSVEVectorSize % 128 == 0 &&
+         "SVE requires vector length in multiples of 128!");
+  assert((MaxSVEVectorSize >= MinSVEVectorSize ||
+          MaxSVEVectorSize == 0) &&
+         "Minimum SVE vector size should not be larger than its maximum!");
----------------
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 `MaxSVEVectorSize`are sane.  Can you do likewise here?


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetMachine.cpp:357
+  Attribute VScaleRangeAttr = F.getFnAttribute(Attribute::VScaleRange);
+  if (VScaleRangeAttr.isValid())
+    std::tie(MinSVEVectorSize, MaxSVEVectorSize) =
----------------
bsmith wrote:
> paulwalker-arm wrote:
> > I don't know if this is possible but I feel we need a `HasSVE` like check here?
> 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.
Fair enough.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103702/new/

https://reviews.llvm.org/D103702