Skip to content

Fix a bug around Vector4.Distance and Sse41.Insert lowering - #81725

Merged
tannergooding merged 4 commits into
dotnet:mainfrom
tannergooding:fix-81585
Feb 7, 2023
Merged

Fix a bug around Vector4.Distance and Sse41.Insert lowering#81725
tannergooding merged 4 commits into
dotnet:mainfrom
tannergooding:fix-81585

Conversation

@tannergooding

Copy link
Copy Markdown
Member

This resolves #81585

There were two root issues at play:

  1. Distance/DistanceSquared had the wrong arg count and so failed importation, resulting in the bug being masked outside of T1
  2. The lowering for Sse41.Insert wasn't accounting for Insert(Insert(x, idx: 3, val: 0), idx: 3, val: nonZero) so the generated mask was incorrect

@ghost ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Feb 6, 2023
@ghost ghost assigned tannergooding Feb 6, 2023
@ghost

ghost commented Feb 6, 2023

Copy link
Copy Markdown

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch, @kunalspathak
See info in area-owners.md if you want to be subscribed.

Issue Details

This resolves #81585

There were two root issues at play:

  1. Distance/DistanceSquared had the wrong arg count and so failed importation, resulting in the bug being masked outside of T1
  2. The lowering for Sse41.Insert wasn't accounting for Insert(Insert(x, idx: 3, val: 0), idx: 3, val: nonZero) so the generated mask was incorrect
Author: tannergooding
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@tannergooding

Copy link
Copy Markdown
Member Author

/azp run runtime-coreclr jitstress-isas-x86, runtime-coreclr libraries-jitstress

@tannergooding
tannergooding marked this pull request as ready for review February 6, 2023 22:53
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).

@tannergooding

Copy link
Copy Markdown
Member Author

CC. @dotnet/jit-contrib, fixes 81725

isa-x86 failures were alrady resolved by #81669

remaining libraries-jitstress is pre-existing but untracked

Assertion failed '(compCurBB->bbMemoryDef & memoryKindSet(ByrefExposed)) != 0' in 'System.Numerics.Tests.PlaneTests:PlaneToStringTest():this' during 'Do value numbering' (IL size 85; hash 0xa819d389; FullOpts)

@tannergooding

Copy link
Copy Markdown
Member Author

Logged #81739 to track the existing untracked issue

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

ssize_t op1Ival = op1Idx->AsIntConCommon()->IconValue();
ival |= (op1Ival & 0x0F);
ival |= ((op1Ival & 0x0F) & ((1 << count_d) - 1));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you plz educate me what's ival here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We have Insert(Insert(vector, data1, op1Ival), data2, ival)

That is, ival is the immediate of the outer Insert and op1Ival is the immediate of the inner Insert.

The ival is broken up into:

zmask: 4-bits
count_d: 2-bits
count_s: 2-bits

The zmask indicates which elements in the result should be set to zero, this overrides any other bits.
The count_d indicates the index of the result that is written using the value at the index of the input (count_s)

So this was taking the zmask from op1IVal and ensuring that the same bits are being zeroed in the new ival.


Given a scenario that looks like

Vector128<float> Example(Vector128<float> input)
{
    return input.WithElement(2, 0.0f)
                .WithElement(3, 1.0f);
}

This ends up lowering down to:

Vector128<float> tmp = Sse41.Insert(input, Vector128<float>.Zero, 0x20);
return Sse41.Insert(tmp, Vector128.Create(1.0f), 0x30);

Since insert of Zero is special, we end up changing this to:

Vector128<float> tmp = Sse41.Insert(input, Vector128<float>.Zero, 0x24); // Zero is contained
return Sse41.Insert(tmp, Vector128.Create(1.0f), 0x30);

We then can further optimize this by folding the two operations into one by combining the two zmasks:

return Sse41.Insert(input, Vector128.Create(1.0f), 0x34);

This allows us to fold any number of WithElement(n, 0.0f) in a chain of Inserts to reduce the number of operations.


The bug here is one where we specifically had:

Vector128<float> Example(Vector128<float> input)
{
    return input.WithElement(3, 0.0f)
                .WithElement(3, 1.0f);
}

Which lowered to:

Vector128<float> tmp = Sse41.Insert(input, Vector128<float>.Zero, 0x30);
return Sse41.Insert(tmp, Vector128.Create(1.0f), 0x30);

And then became:

Which lowered to:
```csharp
Vector128<float> tmp = Sse41.Insert(input, Vector128<float>.Zero, 0x38);
return Sse41.Insert(tmp, Vector128.Create(1.0f), 0x30);

Since we are going from inner: zero, outer: non-zero, we need to handle the case where outer is setting a value that a previous Insert was zeroing and so we need to ensure we take the zmask from op1IVal, but mask off the element we're setting as part of the new ival.

Given we're or'ing the bits together, we'll still preserve any zeroing that ival is doing itself.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That being said, I just realized this should be:

- ival |= ((op1Ival & 0x0F) & ((1 << count_d) - 1));
+ ival |= ((op1Ival & 0x0F) & ~(1 << count_d));

Otherwise we'd mess up for:

Vector128<float> Example(Vector128<float> input)
{
    return input.WithElement(2, 0.0f)
                .WithElement(3, 0.0f)
                .WithElement(2, 1.0f);
}

Since that'd be 0x24, 0x38, and 0x20, which would become 0x3C and 0x20, and then 0x24 when it should become 0x28

@tannergooding
tannergooding merged commit 32ea339 into dotnet:main Feb 7, 2023
@tannergooding
tannergooding deleted the fix-81585 branch February 7, 2023 15:57
@ghost ghost locked as resolved and limited conversation to collaborators Mar 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Test failure System.Numerics.Tests.Vector4Tests.Vector4DistanceTest1

2 participants