ARM64 - Consolidate 'msub' and 'madd' logic - #68363
Conversation
ghost
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT Issue DetailsAs we move forward with implementing more combined operations for ARM64, it is important that we try to make things as simple as we can. I added
|
| @@ -1379,7 +1379,6 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX | |||
| #if defined(TARGET_ARM64) | |||
| void genCodeForJumpCompare(GenTreeOp* tree); | |||
| void genCodeForMadd(GenTreeOp* tree); | |||
There was a problem hiding this comment.
Should madd be handled as well?
There was a problem hiding this comment.
Maybe I should combine this with #68088 .
tannergooding
left a comment
tannergooding
left a comment
There was a problem hiding this comment.
Change makes sense to me and will ensure we don't need to add n new node kinds to eventually handle everything in #68028
It just becomes a case of adding the containment check in lowering and then switching the instruction in codegen, which becomes a much smaller and easier change to get in.
TIHan
changed the title
| if (node->OperIs(GT_ADD) && !node->gtOverflow() && (op1->OperIs(GT_MUL) || op2->OperIs(GT_MUL))) | ||
| // Find "a + b * c" or "a - b * c". | ||
| // Then mark "b * c" op as contained in order to emit 'madd' or 'msub' respectively. | ||
| if (node->OperIs(GT_ADD, GT_SUB) && !(node->gtFlags & GTF_SET_FLAGS) && !node->gtOverflow() && |
There was a problem hiding this comment.
I think I should make some helper functions to handle this; there are a lot of conditions here...
| return next; | ||
| } | ||
| #ifdef TARGET_ARM64 | ||
| if (BlockRange().TryGetUse(node, &use)) |
There was a problem hiding this comment.
What do you need the use for?
There was a problem hiding this comment.
I thought I had an understanding of this, but I think I actually don't.
I removed this piece from the change.
| if (a->OperIs(GT_NEG) && !(a->gtFlags & GTF_SET_FLAGS) && !b->OperIs(GT_NEG) && !a->isContained() && | ||
| !a->gtGetOp1()->isContained()) | ||
| { | ||
| mul->AsOp()->gtOp1 = a->gtGetOp1(); | ||
| BlockRange().Remove(a); | ||
| node->gtOp1 = c; | ||
| node->gtOp2 = mul; | ||
| node->ChangeOper(GT_SUB); | ||
| } | ||
| // Transform "a * -b + c" to "c - a * b" | ||
| else if (b->OperIs(GT_NEG) && !(b->gtFlags & GTF_SET_FLAGS) && !a->OperIs(GT_NEG) && | ||
| !b->isContained() && !b->gtGetOp1()->isContained()) | ||
| { | ||
| mul->AsOp()->gtOp2 = b->gtGetOp1(); | ||
| BlockRange().Remove(b); | ||
| node->gtOp1 = c; | ||
| node->gtOp2 = mul; | ||
| node->ChangeOper(GT_SUB); | ||
| } | ||
| // Transform "a * b + c" to "c + a * b" | ||
| else if (op1->OperIs(GT_MUL)) | ||
| { | ||
| node->gtOp1 = c; | ||
| node->gtOp2 = mul; | ||
| } |
There was a problem hiding this comment.
The reordering that is happening here makes me a little uneasy. Since locals are used at their user, it is now technically possible that this reorders the read of a local with a write of the same local.
There was a problem hiding this comment.
If anything the problem will happen during containment, so this code at least should be ok. And the containment checks are preexisting, so I suppose it is fine in practice.
There was a problem hiding this comment.
I know what you mean, I felt the same way. I spoke to @tannergooding about it.
As far as I know, the re-ordering of the tree itself doesn't change the linear execution unless containment happens - correct me if I'm wrong here... So I think it should be ok.
|
@kunalspathak I think this should do it. I ran an asmdiff locally and it showed no changes. |
|
@dotnet/jit-contrib This PR is done. There should be no diffs with this change. |
4 participants
This PR combines this one: #68363
As we move forward with implementing more combined operations for ARM64, it is important that we try to make things as simple as we can.
I added
GT_MSUB, a lowering-only op for ARM64, that only emitsmsub. However, I think we do not need to introduce a new op to achieve this. We only need to check if the second operand is a multiply op that is contained.