c-rhodes added inline comments.
================
Comment at: clang/lib/Basic/Targets/AArch64.cpp:476-484
+ assert(LangOpts.VScaleMin && "vscale min must be greater than 0!");
+
+ if (LangOpts.VScaleMax)
return std::pair<unsigned, unsigned>(LangOpts.VScaleMin,
LangOpts.VScaleMax);
+
if (hasFeature("sve"))
----------------
paulwalker-arm wrote:
> c-rhodes wrote:
> > paulwalker-arm wrote:
> > > 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.
> > > 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.
> >
> > 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);
> > ```
> >
> >
> 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`.
> 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`.
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`.
> 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`.
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.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D113294/new/
https://reviews.llvm.org/D113294
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
