Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

[Arm64] Implement Simd.Insert#16160

Merged
CarolEidt merged 3 commits into
dotnet:masterfrom
sdmaclea:PR-ARM64-SIMD-INSERT
Feb 15, 2018
Merged

[Arm64] Implement Simd.Insert#16160
CarolEidt merged 3 commits into
dotnet:masterfrom
sdmaclea:PR-ARM64-SIMD-INSERT

Conversation

@sdmaclea
Copy link
Copy Markdown

@sdmaclea sdmaclea commented Feb 1, 2018

@sdmaclea
Copy link
Copy Markdown
Author

sdmaclea commented Feb 1, 2018

This is passing all tests except it hits an ABI issue when returning a Vector64 when the immediate index is out of range.

I will address that in a separate pull request.

@sdmaclea sdmaclea force-pushed the PR-ARM64-SIMD-INSERT branch from 5be2459 to 38c42a5 Compare February 1, 2018 21:09
@sdmaclea
Copy link
Copy Markdown
Author

sdmaclea commented Feb 2, 2018

My test case forgot to check the contained Extract case.

Simd.Insert(x, 2, Simd.Extract(y, 3));

It is failing due to a LSRA issue. Source count does not match consume.

I will amend the patch. When I figure it out.

@tannergooding
Copy link
Copy Markdown
Member

@sdmaclea, I did a bit of work to get HWIntrinsic nodes to support being marked as contained here: #16095

Some of it is applicable to ARM/ARM64 as well.

@sdmaclea
Copy link
Copy Markdown
Author

sdmaclea commented Feb 2, 2018

@tannergooding Thanks. Looks like I need a few lines from #16095. I seems like temporarily disabling the contain code would be prudent.

@sdmaclea sdmaclea force-pushed the PR-ARM64-SIMD-INSERT branch from 38c42a5 to 2a4741b Compare February 9, 2018 21:09
@sdmaclea
Copy link
Copy Markdown
Author

sdmaclea commented Feb 9, 2018

I disabled containment. All tests are passing with the ABI changes which I will push separately.

This should pass regression since previous version also passed.

@CarolEidt PTAL

@sdmaclea sdmaclea force-pushed the PR-ARM64-SIMD-INSERT branch from 2a4741b to cf1e2d8 Compare February 9, 2018 22:53
@sdmaclea
Copy link
Copy Markdown
Author

Pushed a change to re-enable containment now that it is working.

@sdmaclea
Copy link
Copy Markdown
Author

@CarolEidt ping

Copy link
Copy Markdown

@CarolEidt CarolEidt left a comment

Choose a reason for hiding this comment

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

LGTM, but more comments are needed.

Comment thread src/jit/lowerarmarch.cpp
GenTree* op3 = argList->Rest()->Rest()->Current();

// TODO-ARM64-CQ Support containing NI_ARM64_SIMD_GetItem (vector element to element move)
if (op3->OperIs(GT_HWIntrinsic) && (op3->AsHWIntrinsic()->gtHWIntrinsicId == NI_ARM64_SIMD_GetItem))
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

These needs a comment explaining what is being done here (i.e. folding an extract of an element with an insert of that element into another vector).

Comment thread src/jit/codegenarm64.cpp

if (op3->isContained())
{
// Handle vector element to vector element case
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This needs a more explanatory comment to describe at a high level what's being done (e.g. this is a folding of SimdInsertOp(GetItem), and to clarify why 1) op3 is known (asserted) to be a GetItem, and 2) its srcLane is also known to be a constant.

@sdmaclea
Copy link
Copy Markdown
Author

@CarolEidt Added comments per you request

@CarolEidt CarolEidt merged commit cc4ac53 into dotnet:master Feb 15, 2018
@sdmaclea sdmaclea deleted the PR-ARM64-SIMD-INSERT branch May 24, 2018 19:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants