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

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64Subtarget.cpp:220-225
+      MinSVEVectorSizeInBits(MinSVEVectorSizeInBitsOverride
+                                 ? MinSVEVectorSizeInBitsOverride
+                                 : SVEVectorBitsMinOpt),
+      MaxSVEVectorSizeInBits(MaxSVEVectorSizeInBitsOverride
+                                 ? MaxSVEVectorSizeInBitsOverride
+                                 : SVEVectorBitsMaxOpt),
----------------
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.


================
Comment at: llvm/lib/Target/AArch64/AArch64Subtarget.cpp:369-373
+  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!");
----------------
Based on my previous comment I think these two asserts can be move into `AArch64TargetMachine.cpp` also, and/or perhaps `AArch64Subtarget::AArch64Subtarget`.


================
Comment at: llvm/lib/Target/AArch64/AArch64Subtarget.cpp:376
     return 0;
-  return (std::max(SVEVectorBitsMin, SVEVectorBitsMax) / 128) * 128;
+  return (std::max(MinSVEVectorSizeInBits, MaxSVEVectorSizeInBits) / 128) * 128;
 }
----------------
Again assuming my "move opts to AArch64TargetMachine.cpp" suggestion is sound I think this "normalise user input" code can sit in AArch64TargetMachine.cpp also.


================
Comment at: llvm/lib/Target/AArch64/AArch64Subtarget.cpp:381-385
+  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!");
----------------
Same comment as with `getMaxSVEVectorSizeInBits`.


================
Comment at: llvm/lib/Target/AArch64/AArch64Subtarget.cpp:386-388
+  if (MaxSVEVectorSizeInBits == 0)
+    return (MinSVEVectorSizeInBits / 128) * 128;
+  return (std::min(MinSVEVectorSizeInBits, MaxSVEVectorSizeInBits) / 128) * 128;
----------------
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.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetMachine.cpp:357
+  Attribute VScaleRangeAttr = F.getFnAttribute(Attribute::VScaleRange);
+  if (VScaleRangeAttr.isValid())
+    std::tie(MinSVEVectorSize, MaxSVEVectorSize) =
----------------
I don't know if this is possible but I feel we need a `HasSVE` like check here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103702