This is an archive of the discontinued LLVM Phabricator instance.

Differential D107902

[AArch64][SME] Disable NEON in streaming mode
ClosedPublic

Authored by c-rhodes on Aug 11 2021, 7:08 AM.

Details

Reviewers
paulwalker-arm
david-arm
Commits
rG09507b53250d: [AArch64][SME] Disable NEON in streaming mode
Summary

In streaming mode most of the NEON instruction set is illegal, disable
NEON when compiling with +streaming-sve, unless NEON is explictly
requested.

Subsequent patches will add support for the small subset of NEON
instructions that are legal in streaming mode.

Diff Detail

Event Timeline

c-rhodes created this revision.Aug 11 2021, 7:08 AM
Herald added subscribers: ctetreau, hiraditya, kristof.beyls. · View Herald TranscriptAug 11 2021, 7:08 AM
c-rhodes requested review of this revision.Aug 11 2021, 7:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 11 2021, 7:08 AM
c-rhodes added a child revision: D107903: [AArch64][SME] Support NEON vector to GPR integer moves in streaming mode.Aug 11 2021, 7:18 AM
Harbormaster completed remote builds in B119066: Diff 365739.Aug 11 2021, 7:42 AM
Matt added a subscriber: Matt.Aug 11 2021, 9:06 AM
c-rhodes updated this revision to Diff 365960.Aug 12 2021, 4:17 AM

Fix feature.s test, was using dup instruction which aliases mov but the CHECK line checked for dup. Changed to an instruction that doesn't alias to keep it simple.

Harbormaster completed remote builds in B119228: Diff 365960.Aug 12 2021, 4:58 AM
david-arm added inline comments.Aug 13 2021, 1:04 AM
llvm/test/MC/AArch64/SME/feature.s
13 ↗(On Diff #365960)

nit: This comment is a little confusing to be honest! Particularly the bit that says "if streaming-sve (..) is enabled it's disabled". How about something like:

If no CPU is specified the default is generic which implies NEON. However, in streaming-sve mode
NEON is disabled by default, unless the user has explicitly added the feature.

Or something similar?

c-rhodes updated this revision to Diff 366212.Aug 13 2021, 1:16 AM
c-rhodes set the repository for this revision to rG LLVM Github Monorepo.

Clarify comment

c-rhodes marked an inline comment as done.Aug 13 2021, 1:17 AM
c-rhodes added inline comments.
llvm/test/MC/AArch64/SME/feature.s
13 ↗(On Diff #365960)

nit: This comment is a little confusing to be honest! Particularly the bit that says "if streaming-sve (..) is enabled it's disabled". How about something like:

If no CPU is specified the default is generic which implies NEON. However, in streaming-sve mode
NEON is disabled by default, unless the user has explicitly added the feature.

Or something similar?

that's clearer, updated, cheers

Harbormaster completed remote builds in B119408: Diff 366212.Aug 13 2021, 1:55 AM
david-arm accepted this revision.Aug 13 2021, 2:13 AM

LGTM!

This revision is now accepted and ready to land.Aug 13 2021, 2:13 AM
paulwalker-arm added inline comments.Aug 13 2021, 2:53 AM
llvm/lib/Target/AArch64/MCTargetDesc/AArch64MCTargetDesc.cpp
60–69

Does this need to be dependent on CPU="Generic"? I'm thinking you may still need to disable NEON even when targeting a known cpu.

I guess I expected some explicit opting in to the fact NEON will be disabled. That's to say I figured the user would explicitly ask for +streaming-sve and by not also explicitly asking for +neon then they are making a conscious choice to disable NEON.

c-rhodes marked an inline comment as done.Aug 13 2021, 3:08 AM
c-rhodes added inline comments.
llvm/lib/Target/AArch64/MCTargetDesc/AArch64MCTargetDesc.cpp
60–69

Does this need to be dependent on CPU="Generic"? I'm thinking you may still need to disable NEON even when targeting a known cpu.

CPUs with SME support will be defined without FeatureNEON in the features, so this won't be an issue?

I guess I expected some explicit opting in to the fact NEON will be disabled. That's to say I figured the user would explicitly ask for +streaming-sve and by not also explicitly asking for +neon then they are making a conscious choice to disable NEON.

That's what this change implements?

c-rhodes updated this revision to Diff 366240.Aug 13 2021, 4:50 AM
c-rhodes retitled this revision from [AArch64][SME] Disable NEON on generic CPU for streaming mode to [AArch64][SME] Disable NEON in streaming mode.
c-rhodes edited the summary of this revision. (Show Details)

Disable NEON when compiling with +streaming-sve regardless of CPU.

c-rhodes added inline comments.Aug 13 2021, 4:53 AM
llvm/lib/Target/AArch64/MCTargetDesc/AArch64MCTargetDesc.cpp
60–69

Does this need to be dependent on CPU="Generic"? I'm thinking you may still need to disable NEON even when targeting a known cpu.

CPUs with SME support will be defined without FeatureNEON in the features, so this won't be an issue?

This is incorrect, misunderstanding on my part. CPUs with SME support will also have NEON and as such FeatureNEON will be implied.

I guess I expected some explicit opting in to the fact NEON will be disabled. That's to say I figured the user would explicitly ask for +streaming-sve and by not also explicitly asking for +neon then they are making a conscious choice to disable NEON.

That's what this change implements?

I've updated the patch to disable NEON regardless of CPU if +streaming-sve is specified, unless NEON is explicitly requested.

Harbormaster completed remote builds in B119426: Diff 366240.Aug 13 2021, 5:17 AM
paulwalker-arm accepted this revision.Aug 13 2021, 5:26 AM
c-rhodes added a child revision: D108039: [AArch64][SME] Support NEON scalar FP instructions in streaming mode.Aug 13 2021, 7:58 AM
Closed by commit rG09507b53250d: [AArch64][SME] Disable NEON in streaming mode (authored by c-rhodes). · Explain WhyAug 16 2021, 12:57 AM
This revision was automatically updated to reflect the committed changes.
c-rhodes added a commit: rG09507b53250d: [AArch64][SME] Disable NEON in streaming mode.
c-rhodes mentioned this in D108279: [AArch64][SME] Fix v8.6a bf16 NEON instruction predication.Aug 18 2021, 1:41 AM
c-rhodes mentioned this in rGe9c8973f1c68: [AArch64][SME] Fix v8.6a bf16 NEON instruction predication.Aug 24 2021, 1:37 AM

Revision Contents

PathSize
llvm/
lib/
Target/
AArch64/
MCTargetDesc/
11 lines
test/
MC/
AArch64/
SME/
8 lines

Diff 366564

llvm/lib/Target/AArch64/MCTargetDesc/AArch64MCTargetDesc.cpp

Show First 20 Lines • Show All 51 Lines • ▼ Show 20 Lines
createAArch64MCSubtargetInfo(const Triple &TT, StringRef CPU, StringRef FS) { createAArch64MCSubtargetInfo(const Triple &TT, StringRef CPU, StringRef FS) {
if (CPU.empty()) { if (CPU.empty()) {
CPU = "generic"; CPU = "generic";
if (TT.isArm64e()) if (TT.isArm64e())
CPU = "apple-a12"; CPU = "apple-a12";
} }
return createAArch64MCSubtargetInfoImpl(TT, CPU, /*TuneCPU*/ CPU, FS); // Most of the NEON instruction set isn't supported in streaming mode on SME
// targets, disable NEON unless explicitly requested.
bool RequestedNEON = FS.contains("neon");
bool RequestedStreamingSVE = FS.contains("streaming-sve");
MCSubtargetInfo *STI =
createAArch64MCSubtargetInfoImpl(TT, CPU, /*TuneCPU*/ CPU, FS);
if (RequestedStreamingSVE && !RequestedNEON &&
STI->hasFeature(AArch64::FeatureNEON))
STI->ToggleFeature(AArch64::FeatureNEON);
return STI;
paulwalker-armUnsubmitted
Not Done
ReplyInline Actions

Does this need to be dependent on CPU="Generic"? I'm thinking you may still need to disable NEON even when targeting a known cpu.

I guess I expected some explicit opting in to the fact NEON will be disabled. That's to say I figured the user would explicitly ask for +streaming-sve and by not also explicitly asking for +neon then they are making a conscious choice to disable NEON.

paulwalker-arm: Does this need to be dependent on `CPU="Generic"`? I'm thinking you may still need to disable…
c-rhodesAuthorUnsubmitted
Not Done
ReplyInline Actions

Does this need to be dependent on CPU="Generic"? I'm thinking you may still need to disable NEON even when targeting a known cpu.

CPUs with SME support will be defined without FeatureNEON in the features, so this won't be an issue?

I guess I expected some explicit opting in to the fact NEON will be disabled. That's to say I figured the user would explicitly ask for +streaming-sve and by not also explicitly asking for +neon then they are making a conscious choice to disable NEON.

That's what this change implements?

c-rhodes: > Does this need to be dependent on `CPU="Generic"`? I'm thinking you may still need to disable…
c-rhodesAuthorUnsubmitted
Done
ReplyInline Actions

Does this need to be dependent on CPU="Generic"? I'm thinking you may still need to disable NEON even when targeting a known cpu.

CPUs with SME support will be defined without FeatureNEON in the features, so this won't be an issue?

This is incorrect, misunderstanding on my part. CPUs with SME support will also have NEON and as such FeatureNEON will be implied.

I guess I expected some explicit opting in to the fact NEON will be disabled. That's to say I figured the user would explicitly ask for +streaming-sve and by not also explicitly asking for +neon then they are making a conscious choice to disable NEON.

That's what this change implements?

I've updated the patch to disable NEON regardless of CPU if +streaming-sve is specified, unless NEON is explicitly requested.

c-rhodes: > > Does this need to be dependent on `CPU="Generic"`? I'm thinking you may still need to…
} }
void AArch64_MC::initLLVMToCVRegMapping(MCRegisterInfo *MRI) { void AArch64_MC::initLLVMToCVRegMapping(MCRegisterInfo *MRI) {
// Mapping from CodeView to MC register id. // Mapping from CodeView to MC register id.
static const struct { static const struct {
codeview::RegisterId CVReg; codeview::RegisterId CVReg;
MCPhysReg Reg; MCPhysReg Reg;
} RegMap[] = { } RegMap[] = {
▲ Show 20 Lines • Show All 349 Lines • Show Last 20 Lines

llvm/test/MC/AArch64/SME/streaming-sve-feature.s

  • This file was added.
// RUN: llvm-mc -triple=aarch64 -mattr=+streaming-sve,+neon < %s 2>&1 | FileCheck %s
// RUN: not llvm-mc -triple=aarch64 -mattr=+streaming-sve < %s 2>&1 | FileCheck %s --check-prefix=CHECK-ERROR
// Verify NEON is disabled when targeting streaming mode, if it's not
// explicitly requested.
add v0.8b, v1.8b, v2.8b
// CHECK: add v0.8b, v1.8b, v2.8b
// CHECK-ERROR: error: instruction requires: neon