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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits