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

Fix contained LCL_VAR_ADDR in RMW. #50669

Merged
merged 3 commits into from
Apr 8, 2021

Conversation

sandreenko
Copy link
Contributor

Call emitIns_S_I (stack immediate) to generate instruction like:

Generating: N008 (  1,  1) [000005] -c----------         t5 =    CNS_INT   long   123 REG NA $200
                                                              /--*  t4     long
                                                              +--*  t5     long
Generating: N010 (  8,  9) [000006] -c--G-------         t6 = *  ADD       long   REG NA <l:$241, c:$240>
Generating: N012 (  3,  4) [000000] Uc-----N----         t0 =    LCL_VAR_ADDR byref  V00 arg0          NA REG NA
                                                              /--*  t0     byref
                                                              +--*  t6     long
Generating: N014 (???,???) [000012] -A--G-------              *  STOREIND  long   REG NA

IN0001: 000009 add      qword ptr [rsp+08H], 123

Questions that I had:

  1. should not it be fixed in emitHandleMemOp?
    no, because emitHandleMemOp already has an instruction descriptor and it is wrong for this situation because it expects A meaning dereference Address in a register. We could change the fmt inside like we do for:
    // fmt - the instruction format to use. This must be one of the ARD, AWR, or ARW formats. If necessary (such as for
    // GT_CLS_VAR_ADDR), this function will map it to the correct format.

    but it would require a copy-paste of emitIns_S_I into this function.
    Another argument is that we already treat LCL_VAR_ADDR/LCL_FLD_ADDR differently, to show such cases I have changed OperIs(GT_LCL_VAR_ADDR, GT_LCL_FLD_ADDR) to OperIsLocalAddr().

A better long-term solution would be to rewrite emitHandleMemOp to create id but we don't have time for it now.

  1. Do we need to call genUpdateLife for the local?
    It appears not to be necessary because the variable liveness does not change there so we can skip it.
    However, we can do this, as you see 000000 is marked VAR_USEASG so call to genUpdateLife(storeInd) will give us the correct result, UpdateLifeVar just won't do anything with it. I could still add these calls if I hear an opinion that it will be less confusing.

  2. Have I caught all places that need this fix?
    I checked all calls to emitHandleMemOp and found 1 more place that was fixed in this PR, others already have it or don't work with contained LCL_VAR_ADDR.

Fixes #41073 and #49880.

No spmi-diffs.

@sandreenko sandreenko added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Apr 3, 2021
@sandreenko
Copy link
Contributor Author

PTAL @BruceForstall @dotnet/jit-contrib

@sandreenko sandreenko linked an issue Apr 3, 2021 that may be closed by this pull request
@sandreenko sandreenko closed this Apr 3, 2021
@sandreenko sandreenko reopened this Apr 3, 2021
@sandreenko sandreenko merged commit a06eccb into dotnet:main Apr 8, 2021
@JulieLeeMSFT JulieLeeMSFT added this to the 6.0.0 milestone Apr 15, 2021
@sandreenko sandreenko mentioned this pull request May 10, 2021
10 tasks
@ghost ghost locked as resolved and limited conversation to collaborators May 15, 2021
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.

Assertion failed '!memBase->isContained()' contained LCL_VAR_ADDR is generated wrong in RMW.
3 participants