[PATCH] D111790: [AArch64][Driver][SVE] Allow -msve-vector-bits=<n>+ syntax to mean no maximum vscale

[Paul Walker via Phabricator via cfe-commits]

paulwalker-arm added inline comments.


================
Comment at: clang/lib/Basic/Targets/AArch64.cpp:454
 
-  if (Opts.ArmSveVectorBits) {
-    Builder.defineMacro("__ARM_FEATURE_SVE_BITS", Twine(Opts.ArmSveVectorBits));
+  if (Opts.VScaleMin) {
+    Builder.defineMacro("__ARM_FEATURE_SVE_BITS", Twine(Opts.VScaleMin * 128));
----------------
Similar to my previous comment these must only be defined when both `Opts.VScaleMin` and `Opts.VScaleMax` have the same non-zero value.


================
Comment at: clang/lib/Basic/Targets/AArch64.cpp:467
 AArch64TargetInfo::getVScaleRange(const LangOptions &LangOpts) const {
-  if (LangOpts.ArmSveVectorBits) {
-    unsigned VScale = LangOpts.ArmSveVectorBits / 128;
-    return std::pair<unsigned, unsigned>(VScale, VScale);
-  }
+  if (LangOpts.VScaleMin)
+    return std::pair<unsigned, unsigned>(LangOpts.VScaleMin,
----------------
Should this be `LangOpts.VScaleMin || LangOpts.VScaleMax` to be more future proof?


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:1828
+        Val.equals("2048+")) {
+      int Bits = 0;
+      if (Val.endswith("+"))
----------------
I looks like this should be `unsigned` to ensure the correct version of getAsInteger is called.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:1829-1833
+      if (Val.endswith("+"))
+        Val = Val.substr(0, Val.size() - 1);
+      else {
+        bool Invalid = Val.getAsInteger(10, Bits); (void)Invalid;
+        assert(!Invalid && "Failed to parse value");
----------------
I'm wondering if with the extra complexity if it's worth parsing `Val` first before dealing with the numerical values.  For example:

```
if (!Val.equals("scalable")) {
  if (Val.endswith("+")) {
    MinOnly = true;
    Val = Val.substr(0, Val.size() - 1);
  }

  if (Val.getAsInteger(10, Bits) &&
      (Bits == 128 || (Bits == 256...)) {
    Args.MakeArgString("-mvscale-min=", Bits)
    if (!MinOnly)
      Args.MakeArgString("-mvscale-max=", Bits)
  } else
    D.Diag(diag::err_drv_unsupported_option_argument
}
```
After writing this I'm not sure it's actually any better, so just something to consider.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111790