arm64: Add support for BFI and BFX instruction#123138
arm64: Add support for BFI and BFX instruction#123138jonathandavies-arm wants to merge 9 commits intodotnet:mainfrom
Conversation
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
ce512bf to
1a8d58f
Compare
1a8d58f to
83a6004
Compare
|
Spmidiff errors |
- Remove Expect.cs
Fixed |
|
/azp run runtime-coreclr superpmi-replay |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Looks like there are replay failures.
|
| #ifdef TARGET_ARM64 | ||
| case GT_BFX: | ||
| if (operand == this->AsOp()->gtOp1) | ||
| { | ||
| *pUse = &this->AsOp()->gtOp1; | ||
| return true; | ||
| } | ||
| return false; | ||
| #endif |
There was a problem hiding this comment.
Please double check with another node type like GT_AND that this is being added to all the general utilities.
| { | ||
| assert(tree->OperIs(GT_BFI)); | ||
|
|
||
| GenTreeBfm* bfm = tree->AsBfm(); |
There was a problem hiding this comment.
We could just take GenTreeBfm*
| { | ||
| GenTreeBfm* result = new (this, GT_BFI) GenTreeBfm(GT_BFI, type, base, src, offset, width); | ||
| result->gtFlags |= (base->gtFlags | src->gtFlags) & (GTF_ALL_EFFECT); | ||
| result->gtFlags &= ~GTF_SET_FLAGS; |
There was a problem hiding this comment.
| result->gtFlags &= ~GTF_SET_FLAGS; |
Unnecessary?
| { | ||
| GenTreeBfm* result = new (this, GT_BFX) GenTreeBfm(GT_BFX, type, base, nullptr, offset, width); | ||
| result->gtFlags |= (base->gtFlags & GTF_ALL_EFFECT); | ||
| result->gtFlags &= ~GTF_SET_FLAGS; |
There was a problem hiding this comment.
| result->gtFlags &= ~GTF_SET_FLAGS; |
| GTNODE(BFI , GenTreeBfm ,0,0,GTK_BINOP|GTK_EXOP|DBK_NOTHIR) // Bitfield Insert. | ||
| GTNODE(BFIZ , GenTreeOp ,0,0,GTK_BINOP|DBK_NOTHIR) // Bitfield Insert in Zero. | ||
| GTNODE(BFX , GenTreeBfm ,0,0,GTK_UNOP|GTK_EXOP|DBK_NOTHIR) // Bitfield Extract. |
There was a problem hiding this comment.
I assume bfiz is subsumed by bfi? It might be nice to replace uses of one by the other and confirm that you see no regressions.
| baseMaskKnown = false; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
This seems like it needs some interference checks (via IsInvariantInRange). The values being evaluated at the original node locations may change when you are evaluating them in a new node location if you don't check for interference.
| bfm->CopyCosts(tree); | ||
|
|
There was a problem hiding this comment.
| bfm->CopyCosts(tree); |
Don't think that's necessary.
| LIR::Use use; | ||
| if (BlockRange().TryGetUse(tree, &use)) | ||
| { | ||
| use.ReplaceWith(bfm); | ||
| } |
There was a problem hiding this comment.
| LIR::Use use; | |
| if (BlockRange().TryGetUse(tree, &use)) | |
| { | |
| use.ReplaceWith(bfm); | |
| } | |
| LIR::Use use; | |
| if (BlockRange().TryGetUse(tree, &use)) | |
| { | |
| use.ReplaceWith(bfm); | |
| } | |
| else | |
| { | |
| bfm->SetUnusedValue(); | |
| } |
Value-producing nodes without users need to be set as unused. If tree was unused, your transformation is also leaving bfm unused.
| return false; | ||
| } | ||
|
|
||
| BlockRange().Remove(tree); |
There was a problem hiding this comment.
The loop above seems to be able to match more nodes than are being removed here. I would expect that to hit asserts -- you remove some of the nodes and those nodes may have operands that you aren't marking as unused values.
Do you have test cases where the loop above matches a large number of nodes?
This patch adds support for the Arm BFI and BFX instructions. I've added a optimisation in lowering for bit packing and unpacking using each instruction respectively.
BFI
This is used when you pack 2 or more values into an integer. e.g.
return (a & 0xf) | ((b & 0x3) << 4);This is the node pattern it is looking for
and changes to
BFX
When you extract a continuous range of bits from an integer. This is the inverse of above. e.g.
return (x >> 5) & 0x1F;This is the node pattern it's looking for
and changes to