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.

Sign up for GitHub

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

Jump to bottom

Adding support for Vector512 bitwise operations: And, AndNot, Or, OnesComplement, and Xor #83354

Merged
merged 7 commits into from
Mar 16, 2023
Merged

Adding support for Vector512 bitwise operations: And, AndNot, Or, OnesComplement, and Xor #83354

merged 7 commits into from
Mar 16, 2023

Conversation

tannergooding
Copy link
Member

@tannergooding tannergooding commented Mar 13, 2023 •
edited
Loading

This makes progress towards #80814 and #73604

This is dependent on #83402

@tannergooding tannergooding added the avx512 Related to the AVX-512 architecture label Mar 13, 2023
@ghost ghost assigned tannergooding Mar 13, 2023
@dotnet-issue-labeler dotnet-issue-labeler bot added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI new-api-needs-documentation labels Mar 13, 2023
@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost
Copy link

ghost commented Mar 13, 2023

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

Issue Details

This makes progress towards #80814 and #73604

Author: tannergooding
Assignees: tannergooding
Labels:

area-CodeGen-coreclr, new-api-needs-documentation, arch-avx512
Milestone: -

tannergooding
tannergooding commented Mar 13, 2023
View reviewed changes
src/coreclr/jit/emitxarch.h Outdated
Comment on lines 205 to 219
if (IsRexW1Instruction(ins))
{
return true;
}
else if (IsRexWIGInstruction(ins) || IsRexW0Instruction(ins))
{
// TODO: Make this a simple assert once all instructions are annotated
return false;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

I plan on covering this TODO in a follow-up PR (notably will be the "next" thing I do).

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have any systematic way to improve upon the INS_has_wbit flag and the opcode extension bit flag?

I agree that we can move the opcode extension bit into the instruction table, but the TakesRexWBit check is still pretty complicated and requires a lot of contextual information.

If we eventually move everything to checks on instrDesc instead of ins, we probably can simplify this quite a bit. That is later down the road though.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can take a look as part of this. It would indeed be nicer/simpler to be able to handle this "everywhere".

I imagine many of the GPR instructions need something like REX_WX bit that indicates its 0 or 1 based on the EA_ATTR size or alternatively some way to split the instructions between D and Q versions.

Copy link
Member Author

@tannergooding tannergooding Mar 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Have a prototype of tracking the REX.W bit stuff for SIMD via #83473. It should be extensible to GPR as well

tannergooding
tannergooding commented Mar 13, 2023
View reviewed changes
src/coreclr/jit/compiler.cpp Outdated
Comment on lines 2294 to 2298
// We can't use !DoJitStressEvexEncoding() yet because opts.compSupportsISA hasn't
// been set yet as that's what we're trying to set here

if (!JitConfig.JitForceEVEXEncoding() && !JitConfig.JitStressEvexEncoding() &&
!instructionSetFlags.HasInstructionSet(InstructionSet_AVX512F_VL))
Copy link
Member Author

@tannergooding tannergooding Mar 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This was previously !DoJitStressEvexEncoding() and was preventing just JitStressEvexEncoding from working since the support check for AVX512F_VL would always fail.

tannergooding
tannergooding commented Mar 13, 2023
View reviewed changes
src/coreclr/jit/hwintrinsic.cpp
@@ -632,7 +632,7 @@ static bool isSupportedBaseType(NamedIntrinsic intrinsic, CorInfoType baseJitTyp
#ifdef DEBUG
CORINFO_InstructionSet isa = HWIntrinsicInfo::lookupIsa(intrinsic);
#ifdef TARGET_XARCH
assert((isa == InstructionSet_Vector256) || (isa == InstructionSet_Vector128));
assert((isa == InstructionSet_Vector512) || (isa == InstructionSet_Vector256) || (isa == InstructionSet_Vector128));
Copy link
Member Author

Choose a reason for hiding this comment

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

This assert was missing and so we weren't actually testing any InstructionSet_Vector512 paths yet.

tannergooding
tannergooding commented Mar 13, 2023
View reviewed changes
src/coreclr/jit/hwintrinsic.cpp
#if defined(TARGET_ARM64)
if ((simdSize != 8) && (simdSize != 16))
#elif defined(TARGET_XARCH)
if ((simdSize != 16) && (simdSize != 32) && (simdSize != 64))
Copy link
Member Author

Choose a reason for hiding this comment

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

Same for this check against simdSize != 64

This was referenced Mar 14, 2023
Closed
Closed
Closed
Closed
@build-analysis build-analysis bot mentioned this pull request Mar 14, 2023
Closed
@tannergooding tannergooding marked this pull request as ready for review March 14, 2023 16:29
@tannergooding
Copy link
Member Author

CC. @dotnet/jit-contrib, @dotnet/avx512-contrib

@DeepakRajendrakumaran DeepakRajendrakumaran mentioned this pull request Mar 14, 2023
Merged
@tannergooding
Copy link
Member Author

Failures are #83298 which has a revert PR up #83412

@tannergooding tannergooding mentioned this pull request Mar 15, 2023
Merged
@tannergooding
Copy link
Member Author

Rebase to pickup the fix for #83298

EgorBo
EgorBo reviewed Mar 16, 2023
View reviewed changes
src/tests/JIT/HardwareIntrinsics/X86/Avx512F/Program.Avx512F.cs

namespace JIT.HardwareIntrinsics.X86._Avx512F
{
public static partial class Program
Copy link
Member

Choose a reason for hiding this comment

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

Is this how these tests are organized or it's a dead code?

Copy link
Member Author

Choose a reason for hiding this comment

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

Its a stub file that all the generated test projects have at the moment. We could probably remove it, but it's part of the existing "template" so I copied it here as well.

EgorBo
EgorBo approved these changes Mar 16, 2023
View reviewed changes
Copy link
Member

@EgorBo EgorBo left a comment

Choose a reason for hiding this comment

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

LGTM, assuming all tests your changed aren't some outerloop that will break later 🙂
Also I assume for now we don't constant fold any of this ops you just added, right? (presumably, extremely low priority)

@tannergooding
Copy link
Member Author

LGTM, assuming all tests your changed aren't some outerloop that will break later 🙂

They run as part of the runtime-coreclr jitstress-isas-avx512 leg which implicitly lights up for any emitter change.

Also I assume for now we don't constant fold any of this ops you just added, right? (presumably, extremely low priority)

Right, we'll end up going back and adding the constant folding support around the time it gets enabled automatically.

Just rebased onto main to pick up a CI fix so I can merge green.

@build-analysis build-analysis bot mentioned this pull request Mar 16, 2023
Closed
@tannergooding
Copy link
Member Author

tannergooding commented Mar 16, 2023 •
edited
Loading

coreclr linux x64 Debug failures are #83482
mono linux arm64 failures are #81123
browser-wasm linux failures are #80619

Failures in general can be seen to also exist in last scheduled run: https://dev.azure.com/dnceng-public/public/_build/results?buildId=206663&view=ms.vss-test-web.build-test-results-tab&runId=3838833&paneView=debug

@tannergooding tannergooding merged commit 479ccb3 into dotnet:main Mar 16, 2023
@tannergooding tannergooding deleted the avx512-bitwise branch March 16, 2023 20:36
@JulieLeeMSFT JulieLeeMSFT mentioned this pull request Apr 4, 2023
Open
3 tasks
@ghost ghost locked as resolved and limited conversation to collaborators Apr 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@anthonycanino anthonycanino anthonycanino left review comments

@DeepakRajendrakumaran DeepakRajendrakumaran DeepakRajendrakumaran left review comments

@EgorBo EgorBo EgorBo approved these changes

Assignees

@tannergooding tannergooding

Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI avx512 Related to the AVX-512 architecture new-api-needs-documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tannergooding @EgorBo @DeepakRajendrakumaran @anthonycanino