Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[AArch64] NEON, SVE2 and SME2 instruction support with tests #439

Open
wants to merge 72 commits into
base: dev
Choose a base branch
from

Conversation

FinnWilkinson
Copy link
Contributor

This PR adds a wide range of different NEON, SVE2, SME2 instructions with regressions tests. These facilitate a subset of some internal SME-based GEMM and GEMV codes.

There is some BF16 prototypical instruction support which by default is disabled (using a new build option and an if statement in each appropriate switch statement case) due to some usage of __bf16 which is not compiler agnostic, some hacky usage of memcpy to re-interpret uint16_t, and a lack of regression tests for the BF16 instructions in question.

These BF16 instructions can be enabled through a new CMake option -DSIMENG_ENABLE_BF16=ON. I have deliberately not included this in the documentation given the possible instibility of the BF16 implementation and to keep it for (mainly) internal usage only.

This branch is based on sme2-support (PR #429 ) and so should be merged after this brnch has been merged into dev.

Some SM2 instructions which use multi-vector operands can be non-trivial to read or understand. Please ask for clarification and suggest any additional comments that may help future understanding.

@FinnWilkinson FinnWilkinson added the enhancement New feature or request label Nov 4, 2024
@FinnWilkinson FinnWilkinson self-assigned this Nov 4, 2024
@FinnWilkinson FinnWilkinson changed the base branch from dev to sme2-support November 4, 2024 18:12
@FinnWilkinson FinnWilkinson force-pushed the sme-loops-support branch 2 times, most recently from f9a759f to f2b86fa Compare November 7, 2024 19:58
Copy link
Contributor

@ABenC377 ABenC377 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only a few comments

CMakeLists.txt Show resolved Hide resolved
src/include/simeng/arch/aarch64/Instruction.hh Outdated Show resolved Hide resolved
src/include/simeng/arch/aarch64/helpers/sve.hh Outdated Show resolved Hide resolved
@@ -548,7 +549,7 @@ void Instruction::decode() {
} else if (metadata_.operands[0].is_vreg) {
setInstructionType(InsnType::isVectorData);
} else if ((metadata_.operands[0].reg >= AARCH64_REG_ZAB0 &&
metadata_.operands[0].reg <= AARCH64_REG_ZT0) ||
metadata_.operands[0].reg < AARCH64_REG_ZT0) ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can ZT0 be used in a SVE context?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ZT0 is enabled / disabled in the same way as Z0 but has a fixed width of 512-bits, with the logic for detecting whether a ZT0 related instruction can / can't be executed done in instruction_execute as with all other SME instructions.

Regarding where in a core/implementation ZT0 based instructions are executed, there is no fixed rule in the spec as far as I can tell.... Given its fixed width, to me it seems more SVE-like than SME hence the grouping seen here. And given we don't have co-processor SME support, theres no offload / seperate chip logic to come into play yet

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure I follow the answer given here. I was just wondering if the ZT0 is used in SVE instructions, as it seems to be solely used in SME instructions when looking at the spec. If this is the case, would we not want to identify it as an SME instruction? May have missed something though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having gone through the spec more and how ZT0 is used, you're right - it should be SME not SVE ---- Overthinked this a bit previously I think...

src/lib/arch/aarch64/Instruction_execute.cc Outdated Show resolved Hide resolved
test/regression/aarch64/AArch64RegressionTest.hh Outdated Show resolved Hide resolved
test/regression/aarch64/Exception.cc Outdated Show resolved Hide resolved
src/lib/arch/aarch64/Instruction_execute.cc Show resolved Hide resolved
src/lib/arch/aarch64/Instruction_execute.cc Show resolved Hide resolved
src/include/simeng/arch/aarch64/Instruction.hh Outdated Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
ABenC377
ABenC377 previously approved these changes Dec 17, 2024
@FinnWilkinson FinnWilkinson force-pushed the sme-loops-support branch 2 times, most recently from 393dd26 to b027f73 Compare December 18, 2024 15:07
@FinnWilkinson FinnWilkinson changed the base branch from sme2-support to dev December 20, 2024 10:01
@FinnWilkinson FinnWilkinson dismissed ABenC377’s stale review December 20, 2024 10:01

The base branch was changed.

ABenC377
ABenC377 previously approved these changes Dec 20, 2024
// Predicate as counter is 16-bits and has the following encoding:
// - Up to first 4 bits encode the element size (0b1, 0b10, 0b100, 0b1000
// for b h s d respectively)
// - bits 0->LSZ
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LSZ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is terminology from the spec on how predicate-as-counter works. It is LSZ as the number of bits used is dynamic. I'll try find a webpage with this info to better explain it in the comment

// - Bit 15 represents the invert bit
std::array<uint64_t, 4> out = {0, 0, 0, 0};

// Set invert bit to 1 and count to 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add some context for this choice, i.e. relate it back to ptrue. I assume it's because you want to denote true as 0 inactive elements?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, this is how the spec defined it -- will try and find a source to better explain

* W represents how many source elements are multiplied to form an output
* element (i.e. for 4-way, W = 4).
* Returns correctly formatted RegisterValue. */
template <typename D, typename N, int W>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't N always uint8_t and W 4? May have missed something in the docs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will double check and alter if so

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -548,7 +549,7 @@ void Instruction::decode() {
} else if (metadata_.operands[0].is_vreg) {
setInstructionType(InsnType::isVectorData);
} else if ((metadata_.operands[0].reg >= AARCH64_REG_ZAB0 &&
metadata_.operands[0].reg <= AARCH64_REG_ZT0) ||
metadata_.operands[0].reg < AARCH64_REG_ZT0) ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure I follow the answer given here. I was just wondering if the ZT0 is used in SVE instructions, as it seems to be solely used in SME instructions when looking at the spec. If this is the case, would we not want to identify it as an SME instruction? May have missed something though.

@@ -7,8 +7,52 @@ namespace {

using InstSme = AArch64RegressionTest;

#if SIMENG_LLVM_VERSION >= 14
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we not able to keep this check in? I assumed this just concerned no SME and SME support as opposed to SME and SME2 support. May have misremembered the LLVM versioning though

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah you're correct. Will re-add

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Changes Requested
Development

Successfully merging this pull request may close these issues.

3 participants