arm64: Add bic(s) compact encoding #111452
Conversation
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
|
@a74nh @kunalspathak @dotnet/arm64-contrib |
dotnet-policy-service
bot
added
the
community-contribution
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
|
@dotnet-policy-service agree company="Arm" |
src/coreclr/jit/lowerarmarch.cpp
Outdated
| @@ -304,7 +304,12 @@ bool Lowering::IsContainableUnaryOrBinaryOp(GenTree* parentNode, GenTree* childN | |||
| } | |||
| } | |||
|
|
|||
| // TODO: Handle CMN, NEG/NEGS, BIC/BICS, EON, MVN, ORN, TST | |||
| if (childNode->OperIs(GT_LSH) && parentNode->OperIs(GT_AND_NOT)) | |||
There was a problem hiding this comment.
can we leave the TODO comment until all the instructions are handled?
There was a problem hiding this comment.
as mentioned above, we should also not mark as contained if the immediate doesn't fit 6 bits value.
e.g. a & (b << 68) or something.
There was a problem hiding this comment.
I've undo the TODO change.
I've added a couple of tests for bic & bics where the shift value doesn't fit into 6 bits. When you do this it outputs the bic/bics instruction with a shift value of:
output shift value = shift value % max shift amount
where max shift amount is either 32 or 64 depending if you are using 32 or 64 bit registers.
There was a problem hiding this comment.
I'll also add that this behaviour is the same as other instructions such as add, and & sub.
| @@ -19995,7 +19995,7 @@ bool GenTree::SupportsSettingZeroFlag() | |||
| } | |||
| #endif | |||
| #elif defined(TARGET_ARM64) | |||
| if (OperIs(GT_AND)) | |||
| if (OperIs(GT_AND, GT_AND_NOT)) | |||
There was a problem hiding this comment.
any reason why this change was needed? What if we don't do the BIC(s) optimization? Will this still hold true?
| @@ -19995,7 +19995,7 @@ bool GenTree::SupportsSettingZeroFlag() | |||
| } | |||
| #endif | |||
| #elif defined(TARGET_ARM64) | |||
There was a problem hiding this comment.
I was hoping to see code which will check if the shifted amount in not 6-bits and hence cannot be encoded in the instruction, then do not optimize for such cases. Is that not needed?
| static bool BitwiseClearLSR(uint a, uint b, uint c) | ||
| { | ||
| //ARM64-FULL-LINE: bic {{w[0-9]+}}, {{w[0-9]+}}, {{w[0-9]+}}, LSR #4 | ||
| if ((a & ~(b>>4)) == c) |
There was a problem hiding this comment.
can we have test cases where shift value is greater than 64 and make sure that bic is not generated?
There was a problem hiding this comment.
I've added tests but bic is still generated.
| { | ||
| bool fail = false; | ||
|
|
||
| if (Add(1, 2) != 3) |
There was a problem hiding this comment.
Thank you for adding coverage for these, but can we have a separate PR for this?
There was a problem hiding this comment.
I've removed these files and will create a new PR.
Change-Id: Ief95d09c2f7bec412b2509e615f7c09e4d228986
jonathandavies-arm
force-pushed
the
compact-encodings-bic-upstream
branch
from
7853fc5 to
ce3794d
Compare
Contributes to #68028