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

Diff in checked vs. release #98772

Closed
kunalspathak opened this issue Feb 21, 2024 · 1 comment · Fixed by #98789
Closed

Diff in checked vs. release #98772

kunalspathak opened this issue Feb 21, 2024 · 1 comment · Fixed by #98789
Assignees
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI blocking-clean-ci-optional Blocking optional rolling runs
Milestone

Comments

@kunalspathak
Copy link
Member

Call to GetSwitchDescMap() that creates m_switchDescMap if it is nullptr. m_switchDescMap is invalidated everytime we renumber blocks or we replace jump targets. However, if I trace the caller of GetSwitchDescMap(), I see that it can get called from DEBUG via fgDebugCheckProfileWeights() and repopulate the map, which will behave differently at later point of time when we UpdateSwitchTableTarget().

if (m_switchDescMap == nullptr)
{
return; // No mappings, nothing to do.
}

In Debug, map != nullptr and hence we update the target, but in Release, it is still nullptr. In Debug, since we updated the target, it changes the successors we visit and variables created, offsets they get assigned to, etc.

image

Mostly caused by #98526

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Feb 21, 2024
@amanasifkhalid amanasifkhalid added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Feb 21, 2024
@ghost
Copy link

ghost commented Feb 21, 2024

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

Call to GetSwitchDescMap() that creates m_switchDescMap if it is nullptr. m_switchDescMap is invalidated everytime we renumber blocks or we replace jump targets. However, if I trace the caller of GetSwitchDescMap(), I see that it can get called from DEBUG via fgDebugCheckProfileWeights() and repopulate the map, which will behave differently at later point of time when we UpdateSwitchTableTarget().

if (m_switchDescMap == nullptr)
{
return; // No mappings, nothing to do.
}

In Debug, map != nullptr and hence we update the target, but in Release, it is still nullptr. In Debug, since we updated the target, it changes the successors we visit and variables created, offsets they get assigned to, etc.

image

Mostly caused by #98526

Author: kunalspathak
Assignees: amanasifkhalid
Labels:

area-CodeGen-coreclr, untriaged

Milestone: -

@amanasifkhalid amanasifkhalid added the blocking-clean-ci-optional Blocking optional rolling runs label Feb 21, 2024
@amanasifkhalid amanasifkhalid added this to the 9.0.0 milestone Feb 21, 2024
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Feb 21, 2024
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Feb 22, 2024
amanasifkhalid added a commit that referenced this issue Feb 22, 2024
Fixes #98772. Recently, we started checking successor edge likelihoods in Compiler::fgDebugCheckProfileWeights by default; this means we call Compiler::fgDebugCheckOutgoingProfileData in Debug/Checked builds to verify successor edges' likelihoods, and thus call BasicBlock::GetSucc(unsigned, Compiler*) to iterate the successor edges. For switch blocks, GetSucc(unsigned, Compiler*) calls GetSwitchDescMap, and builds m_switchDescMap if it doesn't exist yet. Upon finishing edge likelihood verification, we don't reset m_switchDescMap, so it is possible for this map to be created earlier in Debug/Checked builds versus Release builds.

This doesn't result in behavioral diffs if the map contains the same state between Debug/Release builds by the time it is read. However, if the map is null by the time it is needed, it is created on-demand, ensuring it is completely up-to-date. If the map is not null, the correctness of its current state depends on how judiciously it was maintained with UpdateSwitchTableTarget, creating potential for diffs in the map's state. By invalidating the map instead of updating it as state changes, we can force it to be rebuilt with the latest state when it's needed.
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Feb 22, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Mar 24, 2024
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 blocking-clean-ci-optional Blocking optional rolling runs
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants