This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Add an error if SVE scalable vector types are used in a context without sve
ClosedPublic

Authored by dmgreen on Aug 3 2022, 2:06 AM.

Details

Summary

This adds an error message if the isSVESizelessBuiltinType like __SVFloat32_t / __SVInt64_t / etc, which provide the backing for the svfloat32_t / svint64_t / etc ACLE types, are used in a function without SVE. The alternative is a crash in the backend, which is not capable of handling scalable vector types.

When SVE is available, either through a -march=..+sve option or via a target("sve") attribute, nothing should change. Without the sve feature, this patch gives an error for any function arguments, return values and variable declarations involving the scalable types. Struct/class members and global variables already give an error. (Let me know if there is anything else that should give an error. This might not be all cases, but should rule out the most obvious.) As this can be based on the current function target attributes, the error sometimes needs to be handled later than would otherwise if it was just based on the global target.

This also adds a basic initFeatureMap method for AArch64 to include a few transient sve target dependencies. This should ideally be based on the existing map of features already present in the backend, not reproduced in the target parser.

Diff Detail

Event Timeline

dmgreen created this revision.Aug 3 2022, 2:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 3 2022, 2:06 AM
dmgreen requested review of this revision.Aug 3 2022, 2:06 AM
sdesmalen added inline comments.Aug 3 2022, 5:42 AM
clang/lib/Sema/SemaDecl.cpp
8580

VLSTBuiltinType is synonymous with Vector Length Specific BuiltinType, from the docs:

/// Determines if this is a sizeless type supported by the
/// 'arm_sve_vector_bits' type attribute, which can be applied to a single
/// SVE vector or predicate, excluding tuple types such as svint32x4_t.
bool isVLSTBuiltinType() const;

Additionally, this function doesn't include the tuple types (e.g. svint32x2_t), so if you want this to work for SVE types in general you'll need to create a new method for this.

Matt added a subscriber: Matt.Aug 3 2022, 11:18 AM
dmgreen updated this revision to Diff 449987.Aug 4 2022, 8:17 AM
dmgreen edited the summary of this revision. (Show Details)

Thanks - Added a isSVESizelessBuiltinType function.

sdesmalen added inline comments.Aug 12 2022, 7:48 AM
clang/test/Sema/arm-sve-target.cpp
26

Does this implementation still allow pointers to these types?

For example, it is possible to do:

void foo(struct bar *&ptrA, struct bar *&ptrB) {
    std::swap(ptrA, ptrB);
}

without having defined bar, because it's operating purely on pointers. I would expect the same behaviour for the SVE types, because it doesn't really need SVE to do this operation.

dmgreen added inline comments.Aug 12 2022, 9:01 AM
clang/test/Sema/arm-sve-target.cpp
26

It seems to be fine. It works the same as this, without any extra warnings/errors: https://godbolt.org/z/h978aMPMb

dmgreen updated this revision to Diff 452200.Aug 12 2022, 9:02 AM

Added some pointer tests.

sdesmalen added inline comments.Aug 15 2022, 1:01 AM
clang/lib/Basic/Targets/AArch64.cpp
676

Is it possible to address the FIXME as part of this patch? I'm a bit worried about more technical debt creeping in here, as handling the feature flags is already quite complex and replicated in different places. For example, lib/Driver/ToolChains/Arch/AArch64.cpp has a function DecodeAArch64Features which attempts to do something similar.

dmgreen added inline comments.Aug 16 2022, 8:44 AM
clang/lib/Basic/Targets/AArch64.cpp
676

Ah I see. That is the kind of thing I was hoping to avoid. The backend already knows the map of target features that depend on other features, but it's not available to TargetParser at the moment. We might need to add it in the end, but the entire feature map is really a large problem that cannot be solved very quickly. DecodeAArch64Features looks like quite a small subset of the full dependencies that we'll probably need for feature mapping. See the X86 examples in the Implied features in https://github.com/llvm/llvm-project/blob/main/llvm/lib/Support/X86TargetParser.cpp.

Part of this work will hopefully involve fixing how target(..) attributes work. They currently assume a X86 model, where arch=cpu and there is no cpu=... There is no way (I don't think) to base a target feature off an architecture version, which will be needed for some of the arm_neon.h intrinsics.

So a lot of this will need to change. If you consider the target features not always coming from -march options but alternatively from target(..) attributes then the right place for this is moved away from Driver and into this TargetInfo class. I have changed this slightly, to work with setFeatureEnabled as opposed to initFeatureMap. I was hoping that this would allow the code in DecodeAArch64Features to be removed, but the tests seem to be asserting that +sve2+nosve2 should enable sve, which this would apparently not handle.

dmgreen updated this revision to Diff 453031.Aug 16 2022, 8:46 AM

Update to use setFeatureEnabled.

dmgreen updated this revision to Diff 463495.EditedSep 28 2022, 4:08 AM

Rebase over recent additions. Unfortunately the changes in setFeatureEnabled are required for a number of other patches, as the tests all depend on them. My understanding from speaking to him is that @ilinpv is adding a feature map into the TargetParser, through the Function Multiversioning support in D127812. So the code here in setFeatureEnabled is intended to be temporary, but allows some other patches to move ahead.

dmgreen updated this revision to Diff 464681.Oct 3 2022, 7:29 AM

Rebase over other test changes.

dmgreen updated this revision to Diff 480417.Dec 6 2022, 3:21 AM

Rebase. This requires some dependencies between target features, which is being added in D127812. I would like to get this in sooner rather than later though, to not get too close to a release. There is a simple versions for sve features in this patch. @ilinpv @sdesmalen what do you think?

ilinpv added a comment.Jan 3 2023, 5:31 AM

Rebase. This requires some dependencies between target features, which is being added in D127812. I would like to get this in sooner rather than later though, to not get too close to a release. There is a simple versions for sve features in this patch. @ilinpv @sdesmalen what do you think?

FMV patch with target feautures dependencies relanded fe5cf480ee5a

sdesmalen accepted this revision.Jan 3 2023, 6:24 AM

I'm happy to accept it, but would be good if you can rebase so that the code for inferring target features is no longer necessary.

This revision is now accepted and ready to land.Jan 3 2023, 6:24 AM
This revision was landed with ongoing or failed builds.Jan 12 2023, 10:21 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJan 12 2023, 10:21 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript