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

[libraries-pgo] BBF_HAS_IDX_LEN is not set ... but is required because of the following tree #75827

Closed
jakobbotsch opened this issue Sep 19, 2022 · 7 comments · Fixed by #77176
Assignees
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Milestone

Comments

@jakobbotsch
Copy link
Member

Failure in System.Globalization.Extensions.Tests on multiple Linux configurations.

Work item: https://dev.azure.com/dnceng-public/public/_build/results?buildId=21469&view=ms.vss-test-web.build-test-results-tab&runId=432716&paneView=dotnet-dnceng.dnceng-anon-build-release-tasks.helix-anon-test-information-tab&resultId=192990
Console log: https://helixre107v0xdeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-heads-main-7b6005bf99b2486999/System.Globalization.Extensions.Tests/1/console.8d45794a.log?helixlogtype=result

BBF_HAS_IDX_LEN is not set on BB10 but is required because of the following tree 
N002 (  3,  3) [000126] ---X-------                         *  ARR_LENGTH int   
N001 (  1,  1) [000125] -----------                         \--*  LCL_VAR   ref    V11 tmp8         u:2

Assert failure(PID 21939 [0x000055b3], Thread: 21945 [0x55b9]): Assertion failed 'false' in 'Xunit.Assert:RecordException(System.Func`1[System.Object]):System.Exception' during 'Early Value Propagation' (IL size 48; hash 0xd54f2d8c; Tier1)

    File: /__w/1/s/src/coreclr/jit/earlyprop.cpp Line: 66
    Image: /datadisks/disk1/work/A44E0904/p/dotnet
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Sep 19, 2022
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Sep 19, 2022
@ghost
Copy link

ghost commented Sep 19, 2022

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

Issue Details

Failure in System.Globalization.Extensions.Tests on multiple Linux configurations.

Work item: https://dev.azure.com/dnceng-public/public/_build/results?buildId=21469&view=ms.vss-test-web.build-test-results-tab&runId=432716&paneView=dotnet-dnceng.dnceng-anon-build-release-tasks.helix-anon-test-information-tab&resultId=192990
Console log: https://helixre107v0xdeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-heads-main-7b6005bf99b2486999/System.Globalization.Extensions.Tests/1/console.8d45794a.log?helixlogtype=result

BBF_HAS_IDX_LEN is not set on BB10 but is required because of the following tree 
N002 (  3,  3) [000126] ---X-------                         *  ARR_LENGTH int   
N001 (  1,  1) [000125] -----------                         \--*  LCL_VAR   ref    V11 tmp8         u:2

Assert failure(PID 21939 [0x000055b3], Thread: 21945 [0x55b9]): Assertion failed 'false' in 'Xunit.Assert:RecordException(System.Func`1[System.Object]):System.Exception' during 'Early Value Propagation' (IL size 48; hash 0xd54f2d8c; Tier1)

    File: /__w/1/s/src/coreclr/jit/earlyprop.cpp Line: 66
    Image: /datadisks/disk1/work/A44E0904/p/dotnet
Author: jakobbotsch
Assignees: -
Labels:

area-CodeGen-coreclr, untriaged

Milestone: -

@jakobbotsch jakobbotsch added this to the 8.0.0 milestone Sep 19, 2022
@jakobbotsch jakobbotsch removed the untriaged New issue has not been triaged by the area owner label Sep 19, 2022
@jakobbotsch
Copy link
Member Author

cc @dotnet/jit-contrib

@EgorBo
Copy link
Member

EgorBo commented Sep 19, 2022

@jakobbotsch didn't you want to remove the check in #71707 ? 🙂

@jakobbotsch
Copy link
Member Author

jakobbotsch commented Sep 19, 2022

Yes. But I have run into some regressions in #71707 that I need to think about how to fix before that can be merged.

@jakobbotsch
Copy link
Member Author

But I will assign to myself, it should give me some motivation to work on fixing that. 😉

@jakobbotsch
Copy link
Member Author

#71707 will probably take a while, so we should look at this separately given that it is failing often.

From a preliminary look this is some case of nested inlining not correctly propagating BB flags back.

@jakobbotsch
Copy link
Member Author

At one point here inlining ends up replacing the inline candidate call node with the return expression in-place:

iciCall->ReplaceWith(pInlineInfo->retExpr, this);

The return expression here is itself a GT_RET_EXPR node. This means we end up with multiple copies of that particular node, and the flags are not propagated through the right one.

Not yet sure how to fix it, this is all very convoluted.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Oct 18, 2022
jakobbotsch added a commit that referenced this issue Oct 21, 2022
* Stop using GenTree::ReplaceWith
* GenTreeRetExpr now directly contains its substitution members, which
  is a tree either from the inline candidate, a local (due to spill
  temps) or the call itself (due to inliner failure). In the former
  case, it also contains the BasicBlock the child tree comes from to
  ensure that its flags can be propagated once and for all during
  substitution.
* Make various members strongly typed, e.g.
  GenTreeRetExpr::gtInlineCandidate is now a GenTreeCall*,
  InlineCandidateInfo::retExpr is now a GenTreeRetExpr*. This is
  possible since we are no longer using GenTree::ReplaceWith.

Fix #75827
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Oct 21, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Nov 20, 2022
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 a pull request may close this issue.

2 participants