Page MenuHomePhabricator

[IR] Remove unbounded as possible value for vscale_range minimum
ClosedPublic

Authored by c-rhodes on Nov 5 2021, 9:53 AM.

Details

Summary

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.

Diff Detail

Event Timeline

c-rhodes created this revision.Nov 5 2021, 9:53 AM
c-rhodes requested review of this revision.Nov 5 2021, 9:53 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptNov 5 2021, 9:53 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

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
613

nit: must be an unsigned integer than 0

clang/lib/Frontend/CompilerInvocation.cpp
4128

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
2062

must be greater than 0

c-rhodes updated this revision to Diff 385732.Nov 9 2021, 1:28 AM

Address comments

c-rhodes marked 4 inline comments as done.Nov 9 2021, 1:30 AM

It would be nice if the LLVM interfaces could be updated as well (could also be done in a follow-up patch)

Thanks for reviewing, I'll create a follow-up patch to update the interfaces.

Matt added a subscriber: Matt.Nov 16 2021, 4:12 PM
sdesmalen accepted this revision.Dec 3 2021, 7:34 AM
This revision is now accepted and ready to land.Dec 3 2021, 7:34 AM
paulwalker-arm requested changes to this revision.Dec 3 2021, 9:26 AM
paulwalker-arm added inline comments.
clang/lib/Basic/Targets/AArch64.cpp
476–483

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 revision now requires changes to proceed.Dec 3 2021, 9:26 AM
c-rhodes added inline comments.Dec 3 2021, 9:43 AM
clang/lib/Basic/Targets/AArch64.cpp
476–483

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);

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.

c-rhodes updated this revision to Diff 391668.Dec 3 2021, 10:37 AM

Revert to previous behaviour where both the min/max Clang flags override SVE.

c-rhodes marked an inline comment as done.Dec 3 2021, 10:39 AM

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.

paulwalker-arm added inline comments.Dec 3 2021, 11:12 AM
clang/lib/Basic/Targets/AArch64.cpp
476–483

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.

c-rhodes added inline comments.Dec 6 2021, 1:49 AM
clang/lib/Basic/Targets/AArch64.cpp
476–483

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.

c-rhodes updated this revision to Diff 392002.Dec 6 2021, 2:46 AM

Revert LangOpt.VScaleMin default to 0.

c-rhodes marked an inline comment as done.Dec 6 2021, 2:46 AM
paulwalker-arm accepted this revision.Dec 6 2021, 7:51 AM

One minor issue but otherwise looks good.

clang/lib/Basic/Targets/AArch64.cpp
482

This part is no longer needed because to get here you already know LangOpts.VScaleMin==0 && LangOpts.VScaleMax==0.

This revision is now accepted and ready to land.Dec 6 2021, 7:51 AM
c-rhodes added inline comments.Dec 6 2021, 8:04 AM
clang/lib/Basic/Targets/AArch64.cpp
482

This part is no longer needed because to get here you already know LangOpts.VScaleMin==0 && LangOpts.VScaleMax==0.

Ah of course, I'll fix before committing, cheers!

This revision was landed with ongoing or failed builds.Dec 7 2021, 1:52 AM
This revision was automatically updated to reflect the committed changes.
c-rhodes marked an inline comment as done.Dec 7 2021, 1:54 AM