Take into account the local involved in GT_SWITCH for resolution #97713
Conversation
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
ghost
assigned 
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue Detailsnull
|
src/coreclr/jit/lsra.cpp
Outdated
| #if defined(TARGET_ARM64) || defined(TARGET_LOONGARCH64) || defined(TARGET_RISCV64) || defined(TARGET_X86) | ||
| if (op1->IsLocal()) | ||
| { | ||
| GenTreeLclVarCommon* lcl = op1->AsLclVarCommon(); | ||
| terminatorNodeLclVarDsc = &compiler->lvaTable[lcl->GetLclNum()]; | ||
| } | ||
| #endif |
There was a problem hiding this comment.
Nit: can we merge it with below if to make it consistent with the BBJ_COND case? I.e.
if (op1->OperIs(GT_COPY))
{
// code below
}
else if (op1->IsLocal())
{
// new code
}
Also, I think op2 should get the handling too
There was a problem hiding this comment.
Also, I think op2 should get the handling too
For GT_SWITCH, isn't the op2 always GT_JMPTABLE?
There was a problem hiding this comment.
I think so. But I don't think anything guarantees LSRA doesn't add a GT_COPY on top of it, or something in the future doesn't introduce a local... I think just making it consistent with all the other cases doesn't hurt. In fact we could probably unify all handling here by writing the code something like:
if (block->HasTerminator())
{
LIR::AsRange(block).LastNode()->VisitOperands([](GenTree* op) {
if (op->OperIs(GT_COPY))
{
...
}
else if (op->IsLocal())
{
...
}
return VisitResult::Continue;
});
}which would also be a bit more future proof. But I'm also ok with just keeping the special casing for BBJ_COND/BBJ_SWITCH.
Diff results for #97713Throughput diffsThroughput diffs for osx/arm64 ran on linux/x64MinOpts (-0.01% to +0.00%)
Throughput diffs for windows/arm64 ran on linux/x64MinOpts (-0.00% to +0.01%)
Details here |
No description provided.