-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
arm64: Add support for Bitwise OR NOT & XOR NOT #111893
arm64: Add support for Bitwise OR NOT & XOR NOT #111893
Conversation
jonathandavies-arm
commented
Jan 28, 2025
- contributes towards Review the multi-op instruction usage for Arm64 #68028
* contributes towards dotnet#68028
@a74nh @kunalspathak @dotnet/arm64-contrib |
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good overall. Added some minor comments.
src/coreclr/jit/gentree.cpp
Outdated
@@ -31029,7 +31030,9 @@ bool GenTree::IsVectorPerElementMask(var_types simdBaseType, unsigned simdSize) | |||
case GT_AND: | |||
case GT_AND_NOT: | |||
case GT_OR: | |||
case GT_OR_NOT: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need change here? Isn't this just for base instructions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is checking that a node is a mask. If this node is AND/AND_NOT/OR/XOR
and both of it's children are masks, then it is also a mask. OR_NOT/XOR_NOT
also fit into that pattern. I don't think any hwintrinsics today are going to end up there, but we do have predicate OR_NOT/XOR_NOT
for SVE, so I think we might end up using it later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it.
@@ -9925,7 +9925,7 @@ GenTree* Compiler::fgOptimizeHWIntrinsic(GenTreeHWIntrinsic* node) | |||
genTreeOps oper = actualOper; | |||
|
|||
// We shouldn't find AND_NOT nodes since it should only be produced in lowering | |||
assert(oper != GT_AND_NOT); | |||
assert((oper != GT_AND_NOT) && (oper != GT_OR_NOT) && (oper != GT_XOR_NOT)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update the above comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -8272,7 +8272,7 @@ ValueNum ValueNumStore::EvalHWIntrinsicFunBinary( | |||
if (oper != GT_NONE) | |||
{ | |||
// We shouldn't find AND_NOT nodes since it should only be produced in lowering | |||
assert(oper != GT_AND_NOT); | |||
assert((oper != GT_AND_NOT) && (oper != GT_OR_NOT) && (oper != GT_XOR_NOT)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here. please update the comment in this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
} | ||
|
||
[MethodImpl(MethodImplOptions.NoInlining)] | ||
static uint EonLSLSwap(uint a, uint b) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add one example for int
version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.
@@ -72,7 +72,7 @@ static uint EonLSL(uint a, uint b) | |||
} | |||
|
|||
[MethodImpl(MethodImplOptions.NoInlining)] | |||
static uint EonLSLSwap(uint a, uint b) | |||
static int EonLSLSwap(int a, int b) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i meant in addition to uint
, add a new test for int
. Same for Orn
.
Please resolve the merge conflict and then we should be good to go. |
The current error doesn't seem to be from this PR. |
/azp run runtime |
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for your contribution.
* main: (30 commits) JIT: Optimize bit-wise AND with a constant mask in combination with a left shift in a compare (dotnet#111979) Change how we build the cross-OS DAC to support building in the VMR (dotnet#111927) Add Windows Server 2025 to test configurations (dotnet#111938) [PERF] Move performance testing YAML from dotnet/runtime to dotnet/performance (dotnet#111454) arm64: Add support for Bitwise OR NOT & XOR NOT (dotnet#111893) JIT: Fix cross crossgen comparison failures (dotnet#112078) Bump `StyleCop.Analyzers` to `1.2.0-beta.556` (dotnet#111278) Remove `RequiresProcessIsolation` on InterfaceFolding tests (dotnet#112098) Use hardlinks in helixpublishwitharcade (dotnet#112091) Update breaking change rules regarding byref/objref fields. (dotnet#112087) [daccess] Do not use USE_DAC_TABLE_RVA on Apple platforms (dotnet#112076) use collection syntax in illink (dotnet#108458) Include PDB for all TfmRuntimeSpecificPackageFile (dotnet#111879) [main] Update dependencies from dotnet/emsdk (dotnet#111690) Enable Mono tests (dotnet#111981) Let the debugger knows DATAS is on (dotnet#107115) Tests ran counter (dotnet#111145) Some System.Decimal performance improvements (dotnet#99212) [mono][mini] Remove support for the Xamarin.iOS and Xamarin.Mac assemblies in the AOT compiler. (dotnet#108886) Remove one usage of `Unsafe.AsPointer`. (dotnet#112079) ...