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
