Address comments

Added tests for implicit casts between GNU and fixed-length SVE vectors. I was planning to create a separate patch for this but since it's only tests and no functional changes I've included it in this patch.

In D87463#2267613, @stuij wrote:

I'm not sure what the procedure is to get patches into LLVM 11, but I think there's still time. Could you try to get this in there as well @c-rhodes ?

• Get the element type of scalable vectors from getBuiltinVectorTypeInfo when creating VLS types. This fixes the ABI issue since it calls ASTContext::getIntTypeForBitwidth which gets the correct type for a given target.
• Element type for svfloat16_t is changed from Float16Ty to HalfTy for VLS types since this is what’s used elsewhere.

LGTM

I see no CHECK line for warnings in sve-split-store.ll?

Closing this now the prototype has been split into separate patches that have landed.

In D85743#2244839, @leonardchan wrote:

The IR differences were caused by the new pass manager which is on by default for the Fuchsia builder. I've re-landed the patch with a fix for CodeGen/attr-arm-sve-vector-bits-call.c to use the legacy pm with -fno-experimental-new-pass-manager.

Thanks for the update! We do have the new PM on by default, but I'm surprised that this wouldn't appear on clang-x86_64-debian-new-pass-manager-fast which also tests the new PM.

In D85743#2243188, @c-rhodes wrote:

In D85743#2242931, @leonardchan wrote:

Hi! The attr-arm-sve-vector-bits-call.c test seems to be failing on our clang builders:

Could you take a look? Thanks.

Builder: https://luci-milo.appspot.com/p/fuchsia/builders/ci/clang-linux-x64/b8870800848452818112?

Sorry about that, I've reverted it (commit 2e7041f) whilst I investigate. Thanks for raising.

In D85743#2242931, @leonardchan wrote:

Hi! The attr-arm-sve-vector-bits-call.c test seems to be failing on our clang builders:

Could you take a look? Thanks.

Builder: https://luci-milo.appspot.com/p/fuchsia/builders/ci/clang-linux-x64/b8870800848452818112?

In D85743#2219215, @efriedma wrote:

LGTM

Like I mentioned on the review for the prototype, I still think we should try to implement a scheme that makes CK_BItCast between fixed and scalable types trivial. Doing coercion this way is going to have a significant performance cost. But there isn't any user-visible effect, so I'm fine with leaving that for a followup.

@rsandifo-arm @aaron.ballman thanks for reviewing!

Address comments

ping

LGTM, just one minor nit but seems like a nice improvement

Replace getFixedSize with getKnownMinSize rather than skipping scalable vectors.

More getFixedSize fixes.

Changes:

• s/getKnownMinSize/getFixedSize/g fixes.
• Implemented new mangling scheme for VLS types.

Address @rsandifo-arm comments.

Added missing implicit conversions for C++. I considered handling this with the
existing implicit vector conversion although one side of the conversion will be
an SVE builtin, so instead I've added a new conversion specifically for SVE. I
suspect the existing one could support this but I wasn't sure if that was a good
idea (?). In C++ implicit conversions between VLA/VLS have a rank just below
derived-to-base conversion.

LGTM

In D85725#2211150, @efriedma wrote:

I also tried specifying it in an attribute group but I wasn't sure it was having any effect since it didn't complain when I put garbage in there.

Err, right... aarch64-sve-vector-bits-min is a backend flag; it's not going to have any effect on SROA. (I'm not sure what effect it could have, anyway.) So probably the extra run lines aren't useful.

Specify -aarch64-sve-vector-bits-min=512 in test.

In D85128#2193309, @tschuett wrote:

Sorry. I meant ABI. Can link GCC .o files with Clang .o files using the attributes?

In D85128#2192867, @c-rhodes wrote:

In D85128#2191108, @tschuett wrote:

• Is it compatible with GCC?

Support for this attribute landed in GCC 10 and it's more complete than what this patch implements. We've yet to implement the behaviour guarded by the __ARM_FEATURE_SVE_VECTOR_OPERATORS and __ARM_FEATURE_SVE_PREDICATE_OPERATORS feature macros, so the GNU __attribute__((vector_size)) extension is not available and operators such as binary '+' are not supported for VLSTs. Support for this is intended to be addressed by later patches.

In D85128#2191401, @efriedma wrote:

Not going to write detailed review comments, but this looks like the right approach in general.

Thanks for taking a look! I'll split this up into separate patches soon.

In D85128#2191108, @tschuett wrote:

Stupid questions.

• Is it for convenience? You get arrays, global variables, structs, ... . Vectorization becomes easier ...

I've posted a prototype D85128 with an alternative implementation, given it's quite different to this patch I've posted it as a separate patch and am abandoning this one. See new patch for more details, cheers

there's a few places the getNumElements calls can be fixed by getting the initial cast right

In D83551#2164984, @aaron.ballman wrote:

The attribute bits LGTM, thanks!

• Make helpers static in ASTContext.
• Use getCharWidth.
• s/vector-length-sized/vector-length-specific.

Change the default for EnforceFixedLengthSVEAttribute.

Changes:

• Remove internal type attributes (defined for each vector-size).
• Get the vector size from the arm_sve_vector_bits attribute via the AttributedTypeLoc associated with the typedef decl.
• Change NumBits argument for ArmSveVectorBits type attribute from int to unsigned.
• Only allow ArmSveVectorBits type attribute to be applied to typedefs (and added test).
• Set let PragmaAttributeSupport = 0; after specifying Subjects to fixed clang/test/Misc/pragma-attribute-supported-attributes-list.test.
• vector-length sized -> vector-length-sized.

In D83550#2155837, @aaron.ballman wrote:

LGTM aside from a small nit.

Use const auto * and remove cast

Changes:

• Rebased.
• Added comments for args in calls to ConvertTypeForMem when EnforceFixedLengthSVEAttribute is set and documented EnforceFixedLengthSVEAttribute.
• s/getFixedSVETypeForMemory/getFixedLengthSVETypeForMemory/
• Documented memory representation for fixed-length predicates.

Changes:

• Documented internal type attributes.
• Set ASTNode = 0 on user-facing ArmSveVectorBitsAttr as the internal type attrs are used in the AST. Also removed the case for this from TypePrinter.
• getSveVectorWidth now returns an unsigned. Added an unreachable if T has no attrs.
• s/getArmSveVectorBits/getBitwidthForAttributedSveType. Also now returns an unsigned and asserts if !T->isVLST().
• Add a few comments in test and reduced them a little so we dont tell all types for structs / unions etc.