Fix a couple issues with Vector128.Get/WithElement by tannergooding · Pull Request #52985 · dotnet/runtime

tannergooding · 2021-05-19T18:31:38Z

This resolves #52959 by ensuring that we don't have any unused nodes and that containment checks bail when encountering TYP_SIMD8 or TYP_SIMD12 values where a TYP_SIMD16 or TYP_SIMD32 value is required for containment.
This resolves #52954 by ensuring the expected exception is the one thrown by all Vector indexing
This resolves #53157
This resolves #37260

tannergooding · 2021-05-19T18:32:04Z

CC. @jkotas, @echesakovMSFT

tannergooding · 2021-05-19T19:17:25Z

/azp run runtime-coreclr jitstress-isas-x86

azure-pipelines · 2021-05-19T19:17:40Z

Azure Pipelines successfully started running 1 pipeline(s).

tannergooding · 2021-05-19T21:35:36Z

/azp run runtime-coreclr jitstress-isas-x86, runtime-coreclr jitstress-isas-arm, runtime-coreclr outerloop

azure-pipelines · 2021-05-19T21:36:18Z

Azure Pipelines successfully started running 3 pipeline(s).

jkotas · 2021-05-20T03:51:39Z

Now failing with:

D:\workspace\_work\1\s\src\coreclr\jit\lower.cpp:6025
Assertion failed 'compiler->lvaIsFieldOfDependentlyPromotedStruct(lclVar) || (lclVar->lvSize() == 12)' in 'System.Runtime.Intrinsics.Vector128:AsVector128(System.Numerics.Vector3):System.Runtime.Intrinsics.Vector128`1[System.Single]' during 'Lowering nodeinfo' (IL size 17)

…SE4.1

tannergooding · 2021-05-20T18:59:53Z

Now failing with:

This was because we are cloning a TYP_SIMD12 LclVar in lowering. Lowering "widens" (changes the type) of these to TYP_SIMD16 which causes an assert when the types mismatch on clone.
I resolved this by allowing specifically the TYP_SIMD16 LclVar node over a TYP_SIMD12 backing VarDsc.

There is also a failure in the logic for WithElement in the pre-SSE4.1 path which I've fixed.

tannergooding · 2021-05-20T18:59:58Z

/azp run runtime-coreclr jitstress-isas-x86, runtime-coreclr jitstress-isas-arm, runtime-coreclr outerloop

azure-pipelines · 2021-05-20T19:01:02Z

Azure Pipelines successfully started running 3 pipeline(s).

tannergooding · 2021-05-20T19:39:49Z

/azp run runtime-coreclr jitstress-isas-x86, runtime-coreclr jitstress-isas-arm, runtime-coreclr outerloop

azure-pipelines · 2021-05-20T19:40:34Z

Azure Pipelines successfully started running 3 pipeline(s).

jkotas · 2021-05-20T20:55:36Z

Assertion failed 'compiler->lvaIsFieldOfDependentlyPromotedStruct(lclVar) || (lclVar->lvSize() == 12)' in 'System.Runtime.Intrinsics.Vector128:AsVector128(System.Numerics.Vector3):System.Runtime.Intrinsics.Vector128`1[System.Single]' during 'Lowering nodeinfo' (IL size 17)

I am still seeing this failure even with the latest commit.

tannergooding · 2021-05-20T23:11:07Z

I am still seeing this failure even with the latest commit.

Should be resolved now. Both the LclVar and assignment need to be lowered for ReplaceWithLclVar over TYP_SIMD12 nodes.

tannergooding · 2021-05-20T23:11:37Z

/azp run runtime-coreclr jitstress-isas-x86, runtime-coreclr jitstress-isas-arm, runtime-coreclr outerloop

azure-pipelines · 2021-05-20T23:12:14Z

Azure Pipelines successfully started running 3 pipeline(s).

…en't supported

tannergooding · 2021-05-21T13:54:39Z

/azp run runtime-coreclr jitstress-isas-x86, runtime-coreclr jitstress-isas-arm, runtime-coreclr outerloop

azure-pipelines · 2021-05-21T13:55:17Z

Azure Pipelines successfully started running 3 pipeline(s).

tannergooding · 2021-05-21T18:38:32Z

@jkotas, all tests are passing now. This should be good now.

jkotas · 2021-05-21T18:40:43Z

@jkotas, all tests are passing now. This should be good now.

Thank you. I see all tests passing in runtimelab NativeAOT branch - where I found it initially - as well.

tannergooding · 2021-05-21T18:44:33Z

@echesakovMSFT, @dotnet/jit-contrib can I get a review to unblock the failing jitstress/outerloop tests?

tannergooding · 2021-05-24T19:02:43Z

@echesakovMSFT, @dotnet/jit-contrib can I get a review to unblock the failing jitstress/outerloop tests?

sandreenko

I am not against unblocking outerloop I am just worried it could create more issues, I suggest disabling the failing test in separate PR and take time to finish this.

sandreenko · 2021-05-24T19:16:53Z

+            // We need to lower the LclVar and assignment since there may be certain
+            // types or scenarios, such as TYP_SIMD12, that need special handling
+
+            LowerNode(assign);


I do not think it is a correct approach, this function is used during different phases, for example, during long decomposition, so calling LowerNode during it is incorrect.
ReplaceWithLclVar creates such IR:

currentNode storeCurrentNodeToNewNode NewNodeUse

and lowering works node by node, so after we are done with currentNode we should go to the next and lower it, why don't we?

See, for example, LowerSwitch

runtime/src/coreclr/jit/lower.cpp

Line 504 in b443b8c

ReplaceWithLclVar(use);

we call ReplaceWithLclVar and it creates these new nodes, but then we return node->gtNext that is the created store:

runtime/src/coreclr/jit/lower.cpp

Lines 785 to 791 in b443b8c

GenTree* next = node->gtNext;

// Get rid of the GT_SWITCH(temp).

switchBBRange.Remove(node->AsOp()->gtOp1);

switchBBRange.Remove(node);

return next;

and it does their lowering.

The one used in long decomposition is the instance method exposed on LIR::Use which is distinct from this method that is explicit to lowering and which itself calls into the LIR::Use method

and lowering works node by node, so after we are done with currentNode we should go to the next and lower it, why don't we?

We should already have lowered all the previous nodes, we aren't removing the "current" node, and we aren't inserting a node after the "current" node, so the "next" node is already correct. We simply need to ensure new nodes created and inserted before this node are also lowered.

This is how the existing HWIntrinsic and SimdIntrinsics lowering has been handled and so the same technique is used here as well.

We should already have lowered all the previous nodes, we aren't removing the "current" node,

Agree

and we aren't inserting a node after the "current" node, so the "next" node is already correct.

Don't understand, ReplaceWithLclVar creates new nodes and inserts them after the "current" node, what do you mean?

Don't understand, ReplaceWithLclVar creates new nodes and inserts them after the "current" node, what do you mean?

ReplaceWithLclVar creates new nodes and inserts them after the node it replaces. In this case, we are replacing an operand of the "current" node, which means it precedes the current node in the list and will already have been lowered.

echesakov

I am still reviewing the changes in 1a4e854
But I want to release some comments to speed up the review process

got an explanation

tannergooding · 2021-05-25T01:17:39Z

/azp run runtime-coreclr jitstress-isas-x86, runtime-coreclr jitstress-isas-arm, runtime-coreclr outerloop

azure-pipelines · 2021-05-25T01:18:13Z

Azure Pipelines successfully started running 3 pipeline(s).

echesakov

Looks Good but have some additional questions

ghost · 2021-05-25T17:28:41Z

Hello @tannergooding!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label May 19, 2021

tannergooding force-pushed the fix-52959 branch from c372d09 to 9d6506d Compare May 19, 2021 20:28

tannergooding changed the title ~~Fix an issue with Vector128.WithElement around unused nodes for pre SSE4.1~~ Fix a couple issues with Vector128.Get/WithElement May 19, 2021

clamp03 mentioned this pull request May 20, 2021

Fix structreturn test #53011

Closed

tannergooding added 5 commits May 20, 2021 08:16

Fix an issue with Vector128.WithElement around unused nodes for pre S…

8c2a5b1

…SE4.1

Fixing the expected exception for a structreturn test

241aa5d

Ensure we check if the baseline SIMD ISAs are supported in morph

765d171

Ensure TYP_SIMD12 LclVar can be cloned in lowering

d9bd42e

Fixing up the non SSE41 path for WithElement

1a4e854

runfoapp Bot mentioned this pull request May 20, 2021

readytorun/determinism/crossgen2determinism/crossgen2determinism.sh [FAIL] #50466

Closed

tannergooding force-pushed the fix-52959 branch from f61a0f4 to 1a4e854 Compare May 20, 2021 18:51

runfoapp Bot mentioned this pull request May 20, 2021

InvokeCodeThatShouldFirEvents_EnsureEventsFired fails on OSX #52710

Closed

Applying formatting patch

b7c8926

Ensure ReplaceWithLclVar lowers the created LclVar and assignment

d11612c

tannergooding force-pushed the fix-52959 branch from 9c508f1 to d1192ee Compare May 21, 2021 03:29

Don't check the JitLog for compiled methods when the baseline ISAs ar…

539e7ae

…en't supported

tannergooding force-pushed the fix-52959 branch from d1192ee to 539e7ae Compare May 21, 2021 13:25

This was referenced May 24, 2021

Assertion failed '(genTypeSize(node->TypeGet()) == 16) || (genTypeSize(node->TypeGet()) == 32)' #52959

Closed

Test failure JIT/SIMD/VectorGet_ro/VectorGet_ro.sh #53157

Closed

Assertion failed 'compIsaSupportedDebugOnly(InstructionSet_SSE2)' #37260

Closed

sandreenko previously requested changes May 24, 2021

View reviewed changes

echesakov reviewed May 24, 2021

View reviewed changes

Comment thread src/coreclr/jit/gentree.cpp Outdated

Comment thread src/coreclr/jit/simd.cpp Outdated

Comment thread src/coreclr/jit/lower.h

Address PR feedback

1616c16

echesakov reviewed May 24, 2021

View reviewed changes

Comment thread src/coreclr/jit/lowerxarch.cpp

Comment thread src/tests/JIT/SIMD/VectorGet.cs

Comment thread src/coreclr/jit/lower.h

Comment thread src/coreclr/jit/lowerxarch.cpp Outdated

Comment thread src/coreclr/jit/morph.cpp Outdated

tannergooding added 3 commits May 24, 2021 17:44

Responding to more PR feedback

ff9cc27

Merge remote-tracking branch 'dotnet/main' into fix-52959

ba55519

Applying formatting patch

1b41e6f

echesakov reviewed May 25, 2021

View reviewed changes

Comment thread src/coreclr/jit/morph.cpp Outdated

Comment thread src/coreclr/jit/morph.cpp Outdated

Fixing the more PR review feedback

4f08fc6

echesakov approved these changes May 25, 2021

View reviewed changes

tannergooding added the auto-merge label May 25, 2021

ghost merged commit 5f15498 into dotnet:main May 25, 2021

tannergooding mentioned this pull request May 26, 2021

Tests failed with Assertion failed 'node->IsUnusedValue() && "found an unmarked unused value" #53155

Closed

ghost locked as resolved and limited conversation to collaborators Jun 25, 2021

tannergooding deleted the fix-52959 branch November 11, 2022 15:27

This pull request was closed.

	GenTree* next = node->gtNext;

	// Get rid of the GT_SWITCH(temp).
	switchBBRange.Remove(node->AsOp()->gtOp1);
	switchBBRange.Remove(node);

	return next;

Uh oh!

Conversation

tannergooding commented May 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tannergooding commented May 19, 2021

Uh oh!

tannergooding commented May 19, 2021

Uh oh!

azure-pipelines Bot commented May 19, 2021

Uh oh!

tannergooding commented May 19, 2021

Uh oh!

azure-pipelines Bot commented May 19, 2021

Uh oh!

jkotas commented May 20, 2021

Uh oh!

tannergooding commented May 20, 2021

Uh oh!

tannergooding commented May 20, 2021

Uh oh!

azure-pipelines Bot commented May 20, 2021

Uh oh!

tannergooding commented May 20, 2021

Uh oh!

azure-pipelines Bot commented May 20, 2021

Uh oh!

jkotas commented May 20, 2021

Uh oh!

tannergooding commented May 20, 2021

Uh oh!

tannergooding commented May 20, 2021

Uh oh!

azure-pipelines Bot commented May 20, 2021

Uh oh!

tannergooding commented May 21, 2021

Uh oh!

azure-pipelines Bot commented May 21, 2021

Uh oh!

tannergooding commented May 21, 2021

Uh oh!

jkotas commented May 21, 2021

Uh oh!

tannergooding commented May 21, 2021

Uh oh!

tannergooding commented May 24, 2021

Uh oh!

sandreenko left a comment

Choose a reason for hiding this comment

Uh oh!

sandreenko May 24, 2021

Choose a reason for hiding this comment

Uh oh!

tannergooding May 24, 2021

Choose a reason for hiding this comment

Uh oh!

tannergooding May 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sandreenko May 24, 2021

Choose a reason for hiding this comment

Uh oh!

tannergooding May 24, 2021

Choose a reason for hiding this comment

Uh oh!

echesakov left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tannergooding commented May 25, 2021

Uh oh!

tannergooding commented May 19, 2021 •

edited

Loading

tannergooding May 24, 2021 •

edited

Loading

echesakov left a comment •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.