This is an archive of the discontinued LLVM Phabricator instance.

[PATCH 1/4][Sema][AArch64] Add parsing support for arm_sve_vector_bits attribute
ClosedPublic

Authored by c-rhodes on Jul 10 2020, 6:00 AM.

Details

Summary

This patch implements parsing support for the 'arm_sve_vector_bits' type
attribute, defined by the Arm C Language Extensions (ACLE, version 00bet5,
section 3.7.3) for SVE [1].

The purpose of this attribute is to define fixed-length (VLST) versions
of existing sizeless types (VLAT). For example:

#if __ARM_FEATURE_SVE_BITS==512
typedef svint32_t fixed_svint32_t __attribute__((arm_sve_vector_bits(512)));
#endif

Creates a type 'fixed_svint32_t' that is a fixed-length version of
'svint32_t' that is normal-sized (rather than sizeless) and contains
exactly 512 bits. Unlike 'svint32_t', this type can be used in places
such as structs and arrays where sizeless types can't.

Implemented in this patch is the following:

  • Defined and tested attribute taking single argument.
  • Checks the argument is an integer constant expression.
  • Attribute can only be attached to a single SVE vector or predicate type, excluding tuple types such as svint32x4_t.
  • Added the -msve-vector-bits=<bits> flag. When specified the __ARM_FEATURE_SVE_BITS__EXPERIMENTAL macro is defined.
  • Added a language option to store the vector size specified by the -msve-vector-bits=<bits> flag. This is used to validate N == __ARM_FEATURE_SVE_BITS, where N is the number of bits passed to the attribute and __ARM_FEATURE_SVE_BITS is the feature macro defined under the same flag.

The __ARM_FEATURE_SVE_BITS macro will be made non-experimental in the final
patch of the series.

[1] https://developer.arm.com/documentation/100987/latest

Diff Detail

Event Timeline

c-rhodes created this revision.Jul 10 2020, 6:00 AM
sdesmalen added inline comments.Jul 13 2020, 6:41 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
2816

Do you want to be more specific in the wording here by saying that it doesn't match the value set by -msve-vector-bits?

clang/lib/Sema/SemaType.cpp
7784

I think that the code that checks if the value of the attribute matches the value set by -msve-vector-bits should be part of Sema, not the parser. Also I'm tempted to suggest decoupling the value of the macro from the code that checks the attribute-value matches -msve-vector-bits.

If the arm_sve.h header file defines a constant value like this:

#if defined(__ARM_SVE_FEATURE_BITS)
const unsigned __arm_sve_feature_bits = __ARM_SVE_FEATURE_BITS
#endif

You can check for the availability and value of this constant in the AST during semantic analysis. That way, if for whatever reason the value of the macro is redefined, the compiler can issue a diagnostic. Ideally we would insert a __arm_sve_feature_bits constant into the compilation unit directly when -msve-vector-bits is passed, but I don't know Clang enough to suggest where or at which point to add that.

7795

Should this just be an assert instead? MI should never be nullptr if isMacrodefined(..) returns true.

7809

__ARM_FEATURE_SVE_BITS is never supposed to be set by the user, so the parsing and diagnostic can be simplified, e.g.

if(MI->getNumTokens() != 1 || MI->tokens().front().isNot(tok::numeric_constant)) {
  S.Diag(Attr.getLoc(), diag::err_arm_feature_sve_bits_macro_broken)
  Attr.setInvalid();
  return;
}

but this is probably no longer relevant if you implement the suggestion above on line 7784.

7815

nit: ExprRes is not very descriptive, how about ArmFeatureSveBitsMacroExprRes ?

7818

ArmSveFeatureBits can be nullptr. Or it shouldn't use dyn_cast.

clang/test/Sema/arm-feature-sve-bits-macro.c
1 ↗(On Diff #277009)

nit: -D__ARM_FEATURE_SVE is no longer necessary.

c-rhodes updated this revision to Diff 277853.Jul 14 2020, 8:57 AM

Address @sdesmalen comments

aaron.ballman added inline comments.Jul 14 2020, 12:42 PM
clang/include/clang/Basic/Attr.td
1538

No new, undocumented attributes, please.

clang/include/clang/Basic/DiagnosticSemaKinds.td
2816

Can you wrap this for 80-col limits and put some single quotes around the macro and option names?

2818

Single quotes around the macro name

2820

How about: '__ARM_FEATURE_SVE_BITS' must expand to an integer constant

clang/lib/Sema/SemaType.cpp
7771

This code exists elsewhere in the file as well (see HandleNeonVectorTypeAttr()), can it be factored out into a function and called from both places?

clang/test/Sema/arm-feature-sve-bits-macro.c
3 ↗(On Diff #277853)

This should not be using a system include (unless you control the include path from the RUN line so this doesn't depend on the system running the test).

4 ↗(On Diff #277853)

You should have a test that the attribute works at this location due to the macro being defined on the command line.

22 ↗(On Diff #277853)

I'd also appreciate test cases like:

#define __ARM_FEATURE_SVE_BITS(x)  512
#define __ARM_FEATURE_SVE_BITS N
clang/test/Sema/attr-arm-sve-vector-bits.c
8

Same issue here as above.

c-rhodes marked 6 inline comments as done.Jul 14 2020, 1:36 PM
c-rhodes added inline comments.
clang/lib/Sema/SemaType.cpp
7784

I think that the code that checks if the value of the attribute matches the value set by -msve-vector-bits should be part of Sema, not the parser.

This code which is checking N==__ARM_FEATURE_SVE_BITS is in Sema, maybe there's a more suitable place I'm not aware of but I think it makes sense to check this when looking at the type attribute.

That way, if for whatever reason the value of the macro is redefined, the compiler can issue a diagnostic.

I'm not convinced having a constant in the header fixes that, I suspect the user could redefine that constant as they could the macro, e.g.:

void f() {
  const unsigned __arm_sve_feature_bits = 512;
}

Ideally we want to diagnose inconsistent vector-lengths since we don't support it, but for the time being maybe we can be explicit about what we do support and encourage users to use the -msve-vector-bits flag.

7818

I've added a check + diagnostic if it's a nullptr

clang/test/Sema/arm-feature-sve-bits-macro.c
1 ↗(On Diff #277009)

Missed that one, cheers!

efriedma added inline comments.Jul 14 2020, 2:00 PM
clang/lib/Sema/SemaType.cpp
7784

I don't think it makes sense to try to parse the value of the __ARM_FEATURE_SVE_BITS out of the macro. The macro should be defined by the compiler itself, so we should have the value stored somewhere else.

sdesmalen added inline comments.Jul 14 2020, 2:10 PM
clang/lib/Sema/SemaType.cpp
7784

This code which is checking N==__ARM_FEATURE_SVE_BITS is in Sema, maybe there's a more suitable place I'm not aware of but I think it makes sense to check this when looking at the type attribute.

You're right, I mistook this Sema function for parsing function, my bad.

I'm not convinced having a constant in the header fixes that, I suspect the user could redefine that constant as they could the macro, e.g.:

void f() {
  const unsigned __arm_sve_feature_bits = 512;
}

You can probably search for the value of __arm_sve_feature_bits at a global scope rather than the current scope. It could also be an internal state variable in Clang. As long as we don't have to rely on the value of the macro while we're parsing the file. I'm personally not too bothered if you want to do this in a separate patch while you're still implementing the feature, or if you want to update this patch, but it needs to get fixed because parsing the macro is the wrong way around.

Ideally we want to diagnose inconsistent vector-lengths since we don't support it, but for the time being maybe we can be explicit about what we do support and encourage users to use the -msve-vector-bits flag.

When this feature is implemented, the -msve-vector-bits driver flag is the only interface to set the vector-length, any other way would be invalid.

7818

Thanks.

c-rhodes updated this revision to Diff 278009.Jul 14 2020, 3:56 PM
c-rhodes marked 2 inline comments as done and an inline comment as not done.

Address @aaron.ballman comments

c-rhodes marked 6 inline comments as done.Jul 14 2020, 4:06 PM

@aaron.ballman thanks for comments! I've updated the patch

clang/include/clang/Basic/Attr.td
1538

I've added some docs

clang/test/Sema/arm-feature-sve-bits-macro.c
3 ↗(On Diff #277853)

Good spot, I missed // REQUIRES: aarch64-registered-target which we use in the other ACLE tests.

4 ↗(On Diff #277853)

I think that's covered by clang/test/Sema/attr-arm-sve-vector-bits.c

22 ↗(On Diff #277853)

#define __ARM_FEATURE_SVE_BITS N

Good point, I've added a test for this which I think covers the isValueDependent check.

#define __ARM_FEATURE_SVE_BITS(x) 512

I did try this but no diagnostic is emitted, were you thinking this would cover isTypeDependent? To be honest I copied that from HandleNeonVectorTypeAttr and I'm not sure if it's necessary or what a test would look like for it. The tests for neon_vector_type only cover non-ICE "2.0".

clang/test/Sema/attr-arm-sve-vector-bits.c
8

As above, I've added // REQUIRES: aarch64-registered-target

c-rhodes added inline comments.Jul 14 2020, 4:24 PM
clang/lib/Sema/SemaType.cpp
7784

I don't think it makes sense to try to parse the value of the __ARM_FEATURE_SVE_BITS out of the macro. The macro should be defined by the compiler itself, so we should have the value stored somewhere else.

I intend to set the macro in the patch adding -msve-vector-bits, maybe that flag could set some internal state variable in clang as Sander suggests. I'll look into it.

aaron.ballman added inline comments.Jul 15 2020, 4:59 AM
clang/test/Sema/arm-feature-sve-bits-macro.c
24 ↗(On Diff #278009)

This is a case where I would have expected no diagnostic specifically because N does expand to an integer constant. Is there a reason to not support that?

3 ↗(On Diff #277853)

That makes me uncomfortable as this means you won't get any testing on platforms that may have odd behaviors, like Windows when in MS compatibility mode. I'm not keen on adding attributes that can only be tested under certain circumstances as we want to ensure behavior on all platforms.

We typically handle this by requiring the test to replicate the system header in an Inputs folder that then gets set on the RUN line so that the test can be reproduced on any platform. Was that approach considered and rejected for some reason?

4 ↗(On Diff #277853)

So it is!

22 ↗(On Diff #277853)

Good point, I've added a test for this which I think covers the isValueDependent check.

FWIW, I don't think this covers the value dependent case -- that's more to do with nontype template parameters (I don't believe macros can be type or value dependent; but constant expressions can be).

I did try this but no diagnostic is emitted, were you thinking this would cover isTypeDependent?

Nope, I don't think type dependence factors in (I consider that dependency code to be sanity-checking more than anything; it can probably be assertions if we prefer).

I was under the impression that you only want to support object-like macros and not function-like macros, even if the function-like macro results in an integer constant. If that matters, you can use MacroInfo::isObjectLike() or MacroInfo::isFunctionLike() to test for it.

c-rhodes updated this revision to Diff 278218.Jul 15 2020, 9:44 AM
c-rhodes edited the summary of this revision. (Show Details)

Added -msve-vector-bits=<bits> flag. If specified the __ARM_FEATURE_SVE_BITS__EXPERIMENTAL macro is defined and a language option ArmSveVectorBits is set. The language option replaces the macro parsing in SemaType for the attributed type and the macro tests have been removed. Now that the -msve-vector-bits flag is implemented in this patch, the final patch in the series is intended to make the feature macro non-experimental.

sdesmalen accepted this revision.Jul 15 2020, 10:24 AM

Thanks for the changes @c-rhodes, the use of a LANGOPT for this makes sense to me.
With my nits addressed, I'm happy with the patch if @aaron.ballman is.

clang/include/clang/Basic/AttrDocs.td
4861

nit: Can you add a line here to state this feature is still WIP ? (we can remove that in a later patch)
I know this is implied by your comment re __ARM_FEATURE_SVE_BITS being defined, but I guess it's better to be explicit to avoid confusion.

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

nit: s/__ARM_FEATURE_SVE_BITS__EXPERIMENTAL/__ARM_FEATURE_SVE_BITS_EXPERIMENTAL/ ?

clang/lib/Driver/ToolChains/Clang.cpp
1723

[feel free to ignore] I don't think it necessarily needs to be restricted to this set, it should be able to support any multiple of 128bits. Although if you want to limit it for the first implementation that's fine.

clang/lib/Sema/SemaType.cpp
7726

nit: unrelated change.

7781

nit: can you rename this function to something like verifyValidIntegerConstantExpr to make it clear that that function gives a diagnostic if it fails?

clang/test/Sema/attr-arm-sve-vector-bits.c
2

nit: Given that this test uses -fsyntax-only, can you do the same as you did for clang/test/Driver/aarch64-sve-vector-bits.c and remove aarch64-registered-target and #include <arm_sve.h> and do the tests on the builtin types instead? e.g.

typedef __SVInt32_t svint32_t;
typedef svint32_t fixed_int32_t __attribute__((arm_sve_vector_bits(N)));
This revision is now accepted and ready to land.Jul 15 2020, 10:24 AM
c-rhodes updated this revision to Diff 278249.Jul 15 2020, 11:11 AM
  • Add a note to docs explaining this feature is a WIP.
  • s/__ARM_FEATURE_SVE_BITS__EXPERIMENTAL/__ARM_FEATURE_SVE_BITS_EXPERIMENTAL.
  • s/validIntegerConstantExpr/verifyValidIntegerConstantExpr.
  • Removed unrelated change.
  • Removed // REQUIRES: aarch64-registered-target and #include <arm_sve.h> from test. Sizeless ACLE types are typedef'd in the test, copied from header.
c-rhodes marked 6 inline comments as done.Jul 15 2020, 11:19 AM
c-rhodes added inline comments.
clang/test/Sema/arm-feature-sve-bits-macro.c
3 ↗(On Diff #277853)

That makes me uncomfortable as this means you won't get any testing on platforms that may have odd behaviors, like Windows when in MS compatibility mode. I'm not keen on adding attributes that can only be tested under certain circumstances as we want to ensure behavior on all platforms.

I've removed // REQUIRES: aarch64-registered-target and the include, it's sufficient to typedef the types we need from the header.

We typically handle this by requiring the test to replicate the system header in an Inputs folder that then gets set on the RUN line so that the test can be reproduced on any platform. Was that approach considered and rejected for some reason?

I wasn't aware of this, good to know! That might come in handy in the next patch actually as I use a function from the ACLE to test calls with fixed-length types.

aaron.ballman accepted this revision.Jul 16 2020, 7:57 AM

LGTM aside from a small nit.

clang/lib/Sema/SemaType.cpp
7691

const auto * and you should use cast<> instead of static_cast<>.

Thanks for doing this. FWIW, the patch LGTM from a spec point of view. I just saw one minor spelling nit.

clang/include/clang/Basic/AttrDocs.td
4882

nit: s/behaviour/behavior/, since I think the documentation uses US/international English.

c-rhodes updated this revision to Diff 278507.Jul 16 2020, 9:04 AM

Use const auto * and remove cast

c-rhodes marked an inline comment as done.Jul 16 2020, 9:13 AM

LGTM aside from a small nit.

Thanks for doing this. FWIW, the patch LGTM from a spec point of view. I just saw one minor spelling nit.

Cheers!

clang/include/clang/Basic/AttrDocs.td
4882

nit: s/behaviour/behavior/, since I think the documentation uses US/international English.

Good spot, I'll fix this before merging

clang/lib/Sema/SemaType.cpp
7691

const auto * and you should use cast<> instead of static_cast<>.

I've added const auto *, I don't think the cast was necessary since Attr.getArgAsExpr returns an Expr * although I noticed a few places using static_cast when calling this

This revision was automatically updated to reflect the committed changes.
c-rhodes marked an inline comment as done.
c-rhodes marked an inline comment as done.Jul 17 2020, 3:07 AM