Skip to content

Replace successive "ldr" and "str" instructions with "ldp" and "stp" #77540

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

Merged
kunalspathak merged 30 commits into from
Jan 27, 2023
Merged

Replace successive "ldr" and "str" instructions with "ldp" and "stp" #77540

Changes from 1 commit
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
b88ff31
Replace successive "ldr" and "str" instructions with "ldp" and "stp"
AndyJGraham Sep 6, 2022
f0c918c
No longer use a temporary buffer to build the optimized instruction.
AndyJGraham Oct 31, 2022
f1b236e
Addressed assorted review comments.
AndyJGraham Nov 1, 2022
c0533bd
Now optimizes ascending locations and decending locations with
AndyJGraham Nov 3, 2022
372ee97
Modification to remove last instructions.
AndyJGraham Nov 14, 2022
12fc291
Merge branch 'main'
AndyJGraham Nov 15, 2022
0b377ed
Ongoing improvements to remove previously-emitted instruction
AndyJGraham Nov 29, 2022
46b85f8
Stopped optimization of consecutive instructions that straddled an in…
AndyJGraham Dec 1, 2022
e4741f9
Addressed code change requests in GitHub.
AndyJGraham Dec 1, 2022
2822f64
Merge branch 'main'
AndyJGraham Dec 1, 2022
10a4510
Various fixes to ldp/stp optimization
BruceForstall Dec 2, 2022
d80a69a
Merge pull request #1 from BruceForstall/LdpStp_Modifications_Fixes
AndyJGraham Dec 5, 2022
f6a49bf
Delete unnecessary and incorrect assert
BruceForstall Dec 7, 2022
ed4d070
Merge pull request #2 from BruceForstall/LdpStp_Modifications_FixAsse…
AndyJGraham Dec 7, 2022
4b0e51e
Diagnostic change only, to confirm whether a theory is correct or
AndyJGraham Dec 9, 2022
2997a8e
Revert "Diagnostic change only, to confirm whether a theory is correc…
AndyJGraham Dec 14, 2022
f0907cc
Do not merge. Temporarily removed calls to
AndyJGraham Dec 14, 2022
c5c4234
Modifications to better update the IP mapping table for a replaced in…
AndyJGraham Dec 15, 2022
bb8fdea
Merge branch 'main' of ssh://gerrit.oss.arm.com/enterprise-llt/dotnet…
AndyJGraham Dec 16, 2022
65eed90
Minor formatting change.
AndyJGraham Dec 16, 2022
e03b375
Check for out of range offsets
a74nh Jan 10, 2023
2cef6fc
Don't optimise during prolog/epilog
a74nh Jan 16, 2023
41a9828
Merge branch 'dotnet:main' into LdpStp_Modifications
a74nh Jan 16, 2023
ba89fd3
Fix windows build error
a74nh Jan 16, 2023
1fbf423
Merge branch main
a74nh Jan 19, 2023
ca9a325
IGF_HAS_REMOVED_INSTR is ARM64 only
a74nh Jan 20, 2023
e66ad66
Add OptimizeLdrStr function
a74nh Jan 20, 2023
8b44843
Fix formatting
a74nh Jan 20, 2023
2e7aaf6
Ensure local variables are tracked
a74nh Jan 24, 2023
fe76782
Don't peephole local variables
a74nh Jan 25, 2023
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
Prev Previous commit
Next Next commit
Ensure local variables are tracked
  • Loading branch information
@a74nh
a74nh committed Jan 24, 2023
commit 2e7aaf69f43a4ad6390201b08df6a702a0deea2e
12 changes: 12 additions & 0 deletions src/coreclr/jit/emitarm64.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7684,6 +7684,12 @@ void emitter::emitIns_R_S(instruction ins, emitAttr attr, regNumber reg1, int va
// Try to optimize a load/store with an alternative instruction.
if (isLdrStr && emitComp->opts.OptimizationEnabled() && OptimizeLdrStr(ins, attr, reg1, reg2, imm, size, fmt))
{
// Ensure local variables are tracked.
emitLastIns->idAddr()->iiaLclVar.initLclVarAddr(varx, offs);
Copy link
Contributor

@kunalspathak kunalspathak Jan 24, 2023

Choose a reason for hiding this comment

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

I am wondering emitLastIns would already have this set while processing the previous ldr, right?

Copy link
Contributor

@a74nh a74nh Jan 24, 2023

Choose a reason for hiding this comment

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

For the IsRedundantLdStr case, yes, I think it's essentially the same information in both instructions.

Copy link
Contributor

@kunalspathak kunalspathak Jan 24, 2023

Choose a reason for hiding this comment

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

Correct, so wondering how this would help? What extra information would we track with this that was not already present in emitLastIns?

Copy link
Contributor

@a74nh a74nh Jan 24, 2023

Choose a reason for hiding this comment

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

OptimizeLdrStr() is attempting both IsRedundantLdStr() and ReplaceLdrStrWithPairInstr().

For the IsRedundantLdStr() it's redundant.

For the ReplaceLdrStrWithPairInstr() case (where we are merging two strs into an stp), then the local variable in the first str has already been tracked, but the local variable in the second str needs tracking.

Copy link
Contributor

@kunalspathak kunalspathak Jan 24, 2023

Choose a reason for hiding this comment

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

That makes sense.

emitLastIns->idSetIsLclVar();
#ifdef DEBUG
emitLastIns->idDebugOnlyInfo()->idVarRefOffs = emitVarRefOffs;
#endif
Copy link
Contributor

@BruceForstall BruceForstall Jan 25, 2023

Choose a reason for hiding this comment

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

I don't think this code should live here; it should live in the optimization itself. This code shouldn't be making any assumptions about what the optimization did (and what it didn't do and needs additional fixing up), and the optimization should be more self-contained. Can you pass the varx/offs to the optimization function?

Copy link
Contributor

@a74nh a74nh Jan 25, 2023

Choose a reason for hiding this comment

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

I would propose as a first step, to try and get this PR merged sooner rather than later, to simply not do this optimization if either the previous or current instruction contains lclvar info. Yes, this might remove a lot of the wins. Adding appropriate support for the single or double lclvar info cases could be added back as a follow-up.

Done. Looks like this has removed about 60% of the gains. Which is a shame, but, I agree, let's make this safe and get it merged sooner rather than later.

I'm still allowing the optimisation happen for IsRedundantLdStr(), given that's how it works in HEAD today, so assuming that is safe.

To support this, I think we need a generalized instrDesc that can store 2 separate lclvar infos.

That sounds reasonable, but definitely for a follow on PR.

fwiw, I brought up this problem back in November: #77540 (comment)

Was looking for GC comments yesterday but missed that one. :/

return;
}

Expand Down Expand Up @@ -7917,6 +7923,12 @@ void emitter::emitIns_S_R(instruction ins, emitAttr attr, regNumber reg1, int va
// Try to optimize a store with an alternative instruction.
if (isStr && emitComp->opts.OptimizationEnabled() && OptimizeLdrStr(ins, attr, reg1, reg2, imm, size, fmt))
{
// Ensure local variables are tracked.
emitLastIns->idAddr()->iiaLclVar.initLclVarAddr(varx, offs);
emitLastIns->idSetIsLclVar();
#ifdef DEBUG
emitLastIns->idDebugOnlyInfo()->idVarRefOffs = emitVarRefOffs;
#endif
return;
}

Expand Down