[PATCH] D113294: [IR] Remove unbounded as possible value for vscale_range minimum

[Cullen Rhodes via Phabricator via cfe-commits]

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