The default for min is changed to 1. The behaviour of -mvscale-{min,max}
in Clang is also changed such that 16 is the max vscale when targeting
SVE and no max is specified.
Details
Diff Detail
Event Timeline
It would be nice if the LLVM interfaces could be updated as well (could also be done in a follow-up patch)
From:
std::pair<unsigned, unsigned> getVScaleRangeArgs()
To:
Optional<unsigned> getVScaleRangeMax(); unsigned getVScaleRangeMin();
Since that would simplify some of the queries to this interface.
clang/include/clang/Basic/DiagnosticDriverKinds.td | ||
---|---|---|
597 | nit: must be an unsigned integer than 0 | |
clang/lib/Frontend/CompilerInvocation.cpp | ||
4130 | I'd suggest parsing the value as an integer, and checking that it's > 1. See StringRef::getAsInteger(). | |
llvm/docs/LangRef.rst | ||
2137–2141 | A maximum value of 0 means unbounded. I'd also add that min must be greater than 0. | |
llvm/lib/IR/Verifier.cpp | ||
2064 | must be greater than 0 |
clang/lib/Basic/Targets/AArch64.cpp | ||
---|---|---|
476–484 | This looks like a change of behaviour to me. Previously the command line flags would override the "sve" default but now that only happens when the user specifies a maximum value. That means the interface can no longer be used to force truly width agnostic values. |
clang/lib/Basic/Targets/AArch64.cpp | ||
---|---|---|
476–484 |
I think the issue here is the default of 1 for min would always trigger if (LangOpts.VScaleMin || LangOpts.VScaleMax) overriding the SVE default. Perhaps the default can be removed from the driver option and handled here, i.e. if (LangOpts.VScaleMin || LangOpts.VScaleMax) return std::pair<unsigned, unsigned>(LangOpts.VScaleMin ? LangOpts.VScaleMin : 1, LangOpts.VScaleMax); |
I agree, it's the change to VScaleMin that has caused the issue. If the LangOpts default can remain as 0 and you can still achieve what you're after then that would be perfect.
Not sure why this was an issue when I first looked at it, fresh eyes maybe, fixed now. Thanks for raising.
clang/lib/Basic/Targets/AArch64.cpp | ||
---|---|---|
476–484 | Is this enough? I'm not sure it'll work because LangOpts.VScaleMin defaults to 1 and thus you'll always end up passing the first check, unless the user specifically uses -mvscale-min=0 which they cannot because that'll result in diag::err_cc1_unbounded_vscale_min. Do we need to link the LangOpt defaults to the attribute defaults? I'm think that having the LangOpts default to zero is a good way to represent "value is unspecified" with regards to the LangOpts.VScaleMin. |
clang/lib/Basic/Targets/AArch64.cpp | ||
---|---|---|
476–484 |
It works, I removed the default from the driver option: diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td index 3b85b15739d4..3ec90d42d6ab 100644 --- a/clang/include/clang/Driver/Options.td +++ b/clang/include/clang/Driver/Options.td @@ -3338,7 +3338,7 @@ def msve_vector_bits_EQ : Joined<["-"], "msve-vector-bits=">, Group<m_aarch64_Fe def mvscale_min_EQ : Joined<["-"], "mvscale-min=">, Group<m_aarch64_Features_Group>, Flags<[NoXarchOption,CC1Option]>, HelpText<"Specify the vscale minimum. Defaults to \"1\". (AArch64 only)">, - MarshallingInfoInt<LangOpts<"VScaleMin">, "1">; + MarshallingInfoInt<LangOpts<"VScaleMin">>; and updated clang/test/CodeGen/arm-sve-vector-bits-vscale-range.c.
I'm not sure the LangOpt.VScaleMin default is having any affect, if it were the last CHECK-NONE test in clang/test/CodeGen/arm-sve-vector-bits-vscale-range.c wouldn't pass. I'll revert it back to zero anyway. |
One minor issue but otherwise looks good.
clang/lib/Basic/Targets/AArch64.cpp | ||
---|---|---|
484 | This part is no longer needed because to get here you already know LangOpts.VScaleMin==0 && LangOpts.VScaleMax==0. |
clang/lib/Basic/Targets/AArch64.cpp | ||
---|---|---|
484 |
Ah of course, I'll fix before committing, cheers! |
nit: must be an unsigned integer than 0