Skip to content
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

Arm64: Add back the annotation in JitDisasm with the local var (+offset) #85671

Merged
merged 10 commits into from
May 3, 2023

Conversation

kunalspathak
Copy link
Member

image

Fixes: #84504

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label May 2, 2023
@ghost ghost assigned kunalspathak May 2, 2023
@ghost
Copy link

ghost commented May 2, 2023

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details
image

Fixes: #84504

Author: kunalspathak
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

Comment on lines 609 to +610
unsigned idVarRefOffs; // IL offset for LclVar reference
unsigned idVarRefOffs2; // IL offset for 2nd LclVar reference (in case this is a pair)
Copy link
Contributor

@SingleAccretion SingleAccretion May 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these IL offsets actually maintained/set/used enough to care about them? Last time I checked the logic, that was not the case.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently they are not. The only place they are used currently is in emitDispFrameRef() under a flag compiler->opts.varNames which is always false. Added this to retain the consistency, when (if) we re-enable varNames functionality.

@kunalspathak kunalspathak marked this pull request as ready for review May 2, 2023 22:48
@kunalspathak
Copy link
Member Author

@dotnet/jit-contrib

Copy link
Member

@BruceForstall BruceForstall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. I suppose there can be ambiguity because for pair instructions only one of the pair might be a valid lclvar and when we print it you won't know if it applies to the first or second register?

@@ -1466,6 +1466,7 @@ inline GenTreeLclVar* GenTree::BashToLclVar(Compiler* comp, unsigned lclNum)
ChangeOper(GT_LCL_VAR);
ChangeType(varDsc->lvNormalizeOnLoad() ? varDsc->TypeGet() : genActualType(varDsc));
AsLclVar()->SetLclNum(lclNum);
INDEBUG(AsLclVar()->ResetLclILoffs());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ResetLclILoffs should be done as part of SetOper instead of being scattered around the callers.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought to do in ChangeOper() but that would mean I would have to check for oper == GT_LCL_VAR, but I suppose since this is DEBUG, it shouldn't matter much and may be SetOper() is more natural place. I will move it.

@kunalspathak
Copy link
Member Author

I suppose there can be ambiguity because for pair instructions only one of the pair might be a valid lclvar and when we print it you won't know if it applies to the first or second register?

Not really. If you see the existing code, we would create lclVarPair only if both var1 and var2 are valid.

if (validVar1 && validVar2)
{
id = emitNewInstrLclVarPair(attr, imm);
id->idAddr()->iiaLclVar.initLclVarAddr(varx1, offs1);
id->idSetIsLclVar();
emitGetLclVarPairLclVar2(id)->initLclVarAddr(varx2, offs2);
}

For all the other cases, we will just set IsLclVar() and get the appropriate valid variable information from id->idAddr()->iiaLclVar.

@kunalspathak kunalspathak merged commit c884b11 into dotnet:main May 3, 2023
@kunalspathak kunalspathak deleted the arm64-dasm branch May 3, 2023 15:05
@ghost ghost locked as resolved and limited conversation to collaborators Jun 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Arm64: Add back the annotation in JitDisasm with the local var (+offset)
3 participants