Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
kunalspathak merged 10 commits into from
May 3, 2023
Merged

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

kunalspathak merged 10 commits into from
May 3, 2023

Conversation

@kunalspathak
Copy link
Contributor

@kunalspathak kunalspathak commented May 2, 2023

image

Fixes: #84504

@ghost ghost 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: -

SingleAccretion
SingleAccretion reviewed May 2, 2023
View reviewed changes
src/coreclr/jit/emit.h
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 •
edited
Loading

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
Contributor Author

@kunalspathak kunalspathak May 2, 2023

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.

This was referenced May 2, 2023
Closed
Closed
@kunalspathak kunalspathak marked this pull request as ready for review May 2, 2023 22:48
@kunalspathak kunalspathak requested a review from BruceForstall May 2, 2023 22:48
@kunalspathak
Copy link
Contributor Author

kunalspathak commented May 2, 2023

@dotnet/jit-contrib

BruceForstall
BruceForstall approved these changes May 2, 2023
View reviewed changes
Copy link
Contributor

@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?

SingleAccretion
SingleAccretion reviewed May 2, 2023
View reviewed changes
src/coreclr/jit/compiler.hpp Outdated
ChangeOper(GT_LCL_VAR);
ChangeType(varDsc->lvNormalizeOnLoad() ? varDsc->TypeGet() : genActualType(varDsc));
AsLclVar()->SetLclNum(lclNum);
INDEBUG(AsLclVar()->ResetLclILoffs());
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.

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

Copy link
Contributor Author

@kunalspathak kunalspathak May 3, 2023

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
Contributor Author

kunalspathak commented May 3, 2023

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.

runtime/src/coreclr/jit/emitarm64.cpp

Lines 6577 to 6584 in 56eff9f

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.

@build-analysis build-analysis bot mentioned this pull request May 3, 2023
Closed
@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.

Reviewers

2 more reviewers

@BruceForstall BruceForstall BruceForstall approved these changes

@SingleAccretion SingleAccretion SingleAccretion approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@kunalspathak kunalspathak

Labels

area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

3 participants

@kunalspathak @BruceForstall @SingleAccretion