-
Notifications
You must be signed in to change notification settings - Fork 5k
[APX] fix bugs in NDD code gen #115809
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
base: main
Are you sure you want to change the base?
[APX] fix bugs in NDD code gen #115809
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6420,7 +6420,7 @@ regNumber emitter::emitInsBinary(instruction ins, emitAttr attr, GenTree* dst, G | |
} | ||
else | ||
{ | ||
fmt = useNDD ? emitInsModeFormat(ins, IF_RWR_RRD_ARD) : emitInsModeFormat(ins, IF_RRD_ARD); | ||
fmt = useNDD ? IF_RWR_RRD_ARD : emitInsModeFormat(ins, IF_RRD_ARD); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this because the instruction is annotated as Is this something we should more generally handle or have a centralized helper for? I would imagine it applies to many of the other code paths in here as well, such as for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, the function cannot give the right IF for NDD instructions, so some further code gen optimization failed. Same thing applies to RR, SV, AM. I had to manually set them right. |
||
} | ||
} | ||
else | ||
|
@@ -10572,7 +10572,8 @@ regNumber emitter::emitIns_BASE_R_R_RM( | |
regNumber r = REG_NA; | ||
assert(regOp->isUsedFromReg()); | ||
|
||
if (DoJitUseApxNDD(ins) && regOp->GetRegNum() != targetReg) | ||
if (DoJitUseApxNDD(ins) && regOp->GetRegNum() != targetReg && | ||
!IsRedundantMov(INS_mov, IF_RWR_RRD, attr, targetReg, regOp->GetRegNum(), false)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This probably deserves a comment explaining what What we're basically trying to do here is all the normal handling that More ideally we could do something like the following, that way we can avoid redundant checks in the worst case and keep it understandable: bool useApxNdd = DoJitUseApxNDD(ins);
if (TryEmitIns_Mov(..., useApxNdd) && useApxNdd)
{
// The move is necessary, but we're using APX NDD
return emitInsBinary(ins, attr, regOp, rmOp, targetReg);
}
// The move was either unnecessary or already emitted because we can't use APX NDD
return emitInsBinary(ins, attr, treeNode, rmOp);
if (IsRedundantMov(ins, fmt, attr, dstReg, srcReg, canSkip))
{
- return;
+ // Move is redundant, no need to emit anything
+ return false;
}
if (EmitMovsxAsCwde(ins, size, dstReg, srcReg))
{
- return;
+ // Move is redundant, no need to emit anything
+ return false;
}
+ if (useApxNdd)
+ {
+ // Move is required, but the caller is APX NDD aware
+ return true;
+ }
instrDesc* id = emitNewInstrSmall(attr);
id->idIns(ins);
id->idInsFmt(fmt);
id->idReg1(dstReg);
id->idReg2(srcReg);
UNATIVE_OFFSET sz = emitInsSizeRR(id);
id->idCodeSize(sz);
dispIns(id);
emitCurIGsize += sz;
+ // Move is required and the caller is not APX NDD aware, we emitted the move
+ return true; |
||
{ | ||
r = emitInsBinary(ins, attr, regOp, rmOp, targetReg); | ||
} | ||
|
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.
By directly assigning IF_RWR_RRD_ARD when useNDD is true, ensure that this constant provides equivalent behavior as the previous call to emitInsModeFormat(ins, IF_RWR_RRD_ARD) and doesn't bypass any essential formatting logic.
Copilot uses AI. Check for mistakes.