Avoid reporting empty debug info ranges to the vm #77289
Conversation
ghost
assigned 
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsThis PR is intended to solve issue #47202. The thing is empty variable live ranges are useless. We are reporting them because they can get extended later if the variable becomes alive in the immediately next emitted instruction. If an empty live range is not getting extended, which we can realize after emitting all the code or creating a new live range for the same variable, we can remove it. Tested running before and after this PR, checking the differences manually. Files are big and I wasn't able to get a nice diff that I could post here. I am open to any suggestions for more testing.
|
The debugger is not using empty variable live ranges. We are reporting them because they can get extended later if the variable becomes alive in the immediately next emitted instruction. If an empty live range is not getting extended, which we can realize after emitting all the code or creating a new live range for the same variable, we can remove it.
BrianBohe
force-pushed
the
issue-47202
branch
from
89371b5
to
66d18e0
Compare
|
I am running now R2R dump with BrianBohe@ebb0a45 and found there are still empty variable ranges. Will debug it a little more and open this PR again. |
|
I found a case where a variable (V06) dies (in [000054]) at the beginning of a basic block (BB05), and becomes alive (in [000016]) before an instruction is emitter. We would want to extend the live ranges in this case: |
When the emitter moved to the next group but has not emitted any instruction, and the variable died and becomes alive again, we would like to extend its range.
|
[For last results see last comment] SPMI results show that this approach and its alternative
I was not able to get prints of my code on the log using SPMI locally. But running crossgen to build System.private.corelib dll gets:
Obs: ~10% of variable live ranges are empty after code is generated and could not be omitted before computing emitLocation absolute offsets. On main branch, we are requesting memory to the vm, using it entirely, but reporting empty ranges. Running the same command I got:
Obs: ~15.57% of variable live ranges are empty. This PR introduces a change which allows extending more variable live ranges and replace the ones we know are empty before finishing emitting code, that is why we see a difference in the % of total variable live ranges. |
|
The important aspect of this pr and its alternative approach is to decide whether we want to filter empty live ranges before asking the vm for memory or not. I am moving the updated logic extending the variable range and the removal of duplicated prints to other PRs. After deciding between this PR and its alternative, I will open a new PR to decide whether we want to preemptively remove variable live ranges when we can tell from its instruction group and offset that it is empty, before finishing emitting code. @jakobbotsch pointed me that the memory won't be lost as the vm is taking it, comprising and releasing it. |
|
Update now that we are just comparing both ideas:
Giving the fact that the extra memory is being free after the jit, I see no point on choosing the alternative. Links to this spmi diff and the alternative |
BrianBohe
changed the title
|
Looks like the VM is not happy that we may end up reporting no mappings after asking for memory. I think it should be fine to fix the VM side to handle this correctly. |
|
It looks like crossgen2 will need a small fix to properly free the array: NAOT looks ok. |
|
It might be better to make sure we don't call back into the VM with "valid pointer" + "0 length" combo so that we don't change the contract here. E.g. something like: --- a/src/coreclr/jit/ee_il_dll.cpp
+++ b/src/coreclr/jit/ee_il_dll.cpp
@@ -694,7 +694,19 @@ void Compiler::eeSetLVdone()
}
#endif // DEBUG
- info.compCompHnd->setVars(info.compMethodHnd, eeVarsCount, (ICorDebugInfo::NativeVarInfo*)eeVars);
+ if (eeVarsCount > 0)
+ {
+ info.compCompHnd->setVars(info.compMethodHnd, eeVarsCount, (ICorDebugInfo::NativeVarInfo*)eeVars);
+ }
+ else
+ {
+ info.compCompHnd->setVars(info.compMethodHnd, 0, nullptr);
+
+ if (eeVars != nullptr)
+ {
+ info.compCompHnd->freeArray(eeVars);
+ }
+ }
eeVars = nullptr; // We give up ownership after setVars()
} |
|
@jakobbotsch thanks for the review |
Co-authored-by: Jakob Botsch Nielsen <Jakob.botsch.nielsen@gmail.com>
Would you mind adding the validation that we do not report empty ranges to here? This will make sure it stays correct even when the JIT changes. |
Sure, Who and how is running that function currently? Is there a job scheduled running this once in a while or for every commit in PRs? |
Any pipeline running crossgen2 will also run r2rdump (and therefore that function) |
This PR is intended to solve issue #47202.
The thing is empty variable live ranges are useless. We are reporting them because they can get extended later if the variable becomes alive in the immediately next emitted instruction. If an empty live range is not getting extended, which we can realize after emitting all the code or creating a new live range for the same variable, we can remove it.
Tested running
before and after this PR, checking the differences manually. Files are big and I wasn't able to get a nice diff that I could post here. I am open to any suggestions for more testing.