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.

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

For arm32, kill REG_PROFILER_RET_SCRATCH for LSRA but not for GC #40123

Merged
merged 1 commit into from
Jul 30, 2020
Merged

For arm32, kill REG_PROFILER_RET_SCRATCH for LSRA but not for GC #40123

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/coreclr/src/jit/emit.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7692,7 +7692,12 @@ regMaskTP emitter::emitGetGCRegsKilledByNoGCCall(CorInfoHelpFunc helper)
break;

case CORINFO_HELP_PROF_FCN_LEAVE:
#if defined(TARGET_ARM)
// profiler scratch remains gc live
result = RBM_PROFILER_LEAVE_TRASH & ~RBM_PROFILER_RET_SCRATCH;
#else
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please explain why this is only needed for TARGET_ARM and not other architectures?

Copy link
Member Author

Choose a reason for hiding this comment

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

Arm is the only target that does this sort of return value sheltering in jitted code around the profiler leave hook.

No other architecture defines RBM_PROFILER_RET_SCRATCH.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, thanks

result = RBM_PROFILER_LEAVE_TRASH;
#endif
break;

case CORINFO_HELP_PROF_FCN_TAILCALL:
Expand Down
2 changes: 2 additions & 0 deletions src/coreclr/src/jit/lsraarm.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -491,6 +491,8 @@ int LinearScan::BuildNode(GenTree* tree)

case GT_RETURN:
srcCount = BuildReturn(tree);
killMask = getKillSetForReturn();
BuildDefsWithKills(tree, 0, RBM_NONE, killMask);
Copy link
Contributor

Choose a reason for hiding this comment

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

I am OK with this fix as-is, but I think another option that might be slightly cleaner would be to replace the above 3 lines with:

            buildInternalIntRegisterDefForNode(tree, RBM_PROFILER_ENTER_TRASH);
            srcCount = BuildReturn(tree);
            buildInternalRegisterUses();

Then target.h and emit.cpp could, I believe, remain unchanged. This basically says to the register allocator: "I need an extra register and it must be r2", which will cause it to ensure that r2 is free at the return. However, I haven't tested this approach. Note that normally I wouldn't do this separately (outside of an existing BuildXXX method), but in this case BuildReturn() is shared across platforms and already complex.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I mentioned trying something like that over in the issue but wasn't sure how to pull it off.

Seems like the cleanest solution would be to inject these calls early and handle them like any other call.

Not sure how to proceed -- if you're ok with this fix then approve? Else I can try the above.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll approve; not sure it's worth iterating on.

break;

case GT_RETFILT:
Expand Down
4 changes: 3 additions & 1 deletion src/coreclr/src/jit/target.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1106,7 +1106,9 @@ typedef unsigned char regNumberSmall;
// The registers trashed by profiler enter/leave/tailcall hook
// See vm\arm\asmhelpers.asm for more details.
#define RBM_PROFILER_ENTER_TRASH RBM_NONE
#define RBM_PROFILER_LEAVE_TRASH RBM_NONE
// While REG_PROFILER_RET_SCRATCH is not trashed by the method, the register allocator must
// consider it killed by the return.
#define RBM_PROFILER_LEAVE_TRASH RBM_PROFILER_RET_SCRATCH
#define RBM_PROFILER_TAILCALL_TRASH RBM_NONE

// Which register are int and long values returned in ?
Expand Down