Change gtGetThisArg not to return nullptr. - #44398

Merged
sandreenko merged 3 commits into
dotnet:masterfrom
sandreenko:fixCoreRTConstStr
Nov 19, 2020
Merged

Change gtGetThisArg not to return nullptr.#44398
sandreenko merged 3 commits into
dotnet:masterfrom
sandreenko:fixCoreRTConstStr

Conversation

@sandreenko

@sandreenkosandreenko commented Nov 9, 2020•
edited
Loading

Copy link
Copy Markdown
Contributor

bb0c5dc: Don't wrap string literal const as nop for CoreRT.

It was probably an old workaround for another Jit bug, it is most likely fixed by now.

2ae10db1383: Change gtGetThisArg not to return nullptr.

There was only 1 case where a null return was tolerated - for a tail call marked as virtual in optAssertionGen.
Check this case before we call gtGetThisArg.

Fixesdotnet/runtimelab#298

@sandreenkosandreenko added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Nov 9, 2020
sandreenko
sandreenko commented Nov 9, 2020
View reviewed changes
Comment threadsrc/coreclr/src/jit/assertionprop.cpp Outdated

@sandreenkosandreenkoNov 9, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

In fgMorphTailCallViaJitHelper we clear NeedsNullCheck, should not we also clear IsVirtual?
Then this condition will be just if (call->NeedsNullCheck() || call->IsVirtual())

@sandreenkosandreenkoNov 9, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@jakobbotsch could you please advise me here?

@jakobbotschjakobbotschNov 9, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sorry I don't know. fgMorphTailCallViaJitHelper is the old (from before #341) tailcall mechanism used only on x86. This mechanism might be used directly for virtual calls (or VSDs), but I don't remember how the address is computed in those cases.

@sandreenko

sandreenko commented Nov 9, 2020

Copy link
Copy Markdown
ContributorAuthor

PTAL @dotnet/jit-contrib

@sandreenko

sandreenko commented Nov 9, 2020

Copy link
Copy Markdown
ContributorAuthor

/azp run runtime-coreclr outerloop

@azure-pipelines

azure-pipelinesBot commented Nov 9, 2020

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

sandreenko
sandreenko commented Nov 9, 2020
View reviewed changes
Comment threadsrc/coreclr/src/jit/morph.cpp Outdated

@sandreenkosandreenkoNov 9, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

We do it for stub and in my understanding for vtable we should do the same, let's see what ci thinks.

@jakobbotschjakobbotschNov 9, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is fgMorphTailCallViaHelpers which is for the new mechanism. Indeed those turn into normal direct calls always.

@sandreenkosandreenkoNov 9, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

it looks like I will need the same fix for fgMorphTailCallViaJitHelper: sandreenko@53cea4e , but I want to see if our ci catches it.

@jakobbotschjakobbotschNov 9, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It looks like we remove all virtual kinds below:

runtime/src/coreclr/src/jit/morph.cpp

Lines 7917 to 7921 in dc3101b

// This is now a direct call to the store args stub and not a tailcall.
call->gtCallType = CT_USER_FUNC;
call->gtCallMethHnd = help.hStoreArgs;
call->gtFlags &= ~GTF_CALL_VIRT_KIND_MASK;
call->gtCallMoreFlags &= ~(GTF_CALL_M_TAILCALL | GTF_CALL_M_DELEGATE_INV | GTF_CALL_M_WRAPPER_DELEGATE_INV);

I guess this line is not required but doesn't hurt since this is where we remove the arg for virtual stubs.

@sandreenkosandreenkoNov 10, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I see, thank you. I removed changes in fgMorphTailCallViaHelpers and fixed fgMorphTailCallViaJitHelper in the same way.

Sergey Andreenko
Don't wrap string literal const as nop for CoreRT.
bb0c5dc
It was probably an old workaround for another Jit bug, it is most likely fixed by now.
@sandreenkosandreenko changed the title Fix CoreRT const string handling.Change gtGetThisArg not to return nullptr.Nov 10, 2020
Sergey Andreenko added 2 commits November 10, 2020 10:58
Sergey Andreenko
Change gtGetThisArg not to return nullptr.
007f31f
There was only 1 case where a null return was tolerated - for a tail call in `optAssertionGen` marked as virtual.
However, a transformed tail call is never a virtual, fix `fgMorphTailCallViaJitHelper` to unset virtual flag.
Sergey Andreenko
Keep x86 tail call via jit helper as a virtual stub call.
8663da9
@sandreenko

sandreenko commented Nov 10, 2020

Copy link
Copy Markdown
ContributorAuthor

/azp run runtime-coreclr outerloop

@azure-pipelines

azure-pipelinesBot commented Nov 10, 2020

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

sandreenko
sandreenko commented Nov 11, 2020
View reviewed changes
Comment threadsrc/coreclr/src/jit/morph.cpp
thisPtr = objp;
}

// TODO-Cleanup: we leave it as a virtual stub call to

@sandreenkosandreenkoNov 11, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I made a try to clear GTF_CALL_VIRT_KIND_MASK here but it caused failures because logic in LowerCall depends on this flag to be set in some cases, so I reverted that change.

It would be nice to clear it and fix the failures but it is out of the scope of this bug fix for CoreRT.

@runfoapprunfoappBot mentioned this pull request Nov 11, 2020
Closed
@sandreenko

sandreenko commented Nov 14, 2020

Copy link
Copy Markdown
ContributorAuthor

ping @dotnet/jit-contrib

@sandreenko

sandreenko commented Nov 19, 2020

Copy link
Copy Markdown
ContributorAuthor

Thanks @BruceForstall for the review!

@sandreenko
sandreenko merged commit 044ee8c into dotnet:masterNov 19, 2020
@jkotasjkotas mentioned this pull request Nov 21, 2020
Merged
@sandreenkosandreenko mentioned this pull request Nov 23, 2020
Merged
@ghostghost locked as resolved and limited conversation to collaborators Dec 19, 2020
@sandreenko
sandreenko deleted the fixCoreRTConstStr branch May 17, 2021 07:50
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jakobbotschjakobbotschjakobbotsch left review comments
+1 more reviewer
@BruceForstallBruceForstallBruceForstall approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Assertion failed 'op1 != nullptr' in 'TestUtil.TestLog:VerifyOutput():int:this' during 'Morph - Global' (IL size 130)

3 participants

@sandreenko@jakobbotsch@BruceForstall
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
Change gtGetThisArg not to return nullptr. - #44398
Merged
sandreenko merged 3 commits into
dotnet:masterfrom 
sandreenko:fixCoreRTConstStr
Nov 19, 2020
Merged
Change gtGetThisArg not to return nullptr.#44398
sandreenko merged 3 commits into
dotnet:masterfrom 
sandreenko:fixCoreRTConstStr
Conversation
@sandreenko
@sandreenkosandreenko
commented
Nov 9, 2020•

edited

Loading

Copy link


Copy Markdown

Contributor
bb0c5dc: Don't wrap string literal const as nop for CoreRT.
It was probably an old workaround for another Jit bug, it is most likely fixed by now.
2ae10db1383: Change gtGetThisArg not to return nullptr.
There was only 1 case where a null return was tolerated - for a tail call marked as virtual in optAssertionGen.

Check this case before we call gtGetThisArg.
Fixesdotnet/runtimelab#298
@sandreenkosandreenko
added
the 
area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
label
Nov 9, 2020
sandreenko
sandreenko
commented
Nov 9, 2020
View reviewed changes
Comment threadsrc/coreclr/src/jit/assertionprop.cpp
Outdated

@sandreenkosandreenkoNov 9, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

In fgMorphTailCallViaJitHelper we clear NeedsNullCheck, should not we also clear IsVirtual?

Then this condition will be just if (call->NeedsNullCheck() || call->IsVirtual())
@sandreenkosandreenkoNov 9, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

@jakobbotsch could you please advise me here?
@jakobbotschjakobbotschNov 9, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Sorry I don't know. fgMorphTailCallViaJitHelper is the old (from before #341) tailcall mechanism used only on x86. This mechanism might be used directly for virtual calls (or VSDs), but I don't remember how the address is computed in those cases.
@sandreenko
sandreenko
commented
Nov 9, 2020

Copy link


Copy Markdown

ContributorAuthor
PTAL @dotnet/jit-contrib
@sandreenko
sandreenko
commented
Nov 9, 2020

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Nov 9, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
sandreenko
sandreenko
commented
Nov 9, 2020
View reviewed changes
Comment threadsrc/coreclr/src/jit/morph.cpp
Outdated

@sandreenkosandreenkoNov 9, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

We do it for stub and in my understanding for vtable we should do the same, let's see what ci thinks.
@jakobbotschjakobbotschNov 9, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This is fgMorphTailCallViaHelpers which is for the new mechanism. Indeed those turn into normal direct calls always.
@sandreenkosandreenkoNov 9, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

it looks like I will need the same fix for fgMorphTailCallViaJitHelper: sandreenko@53cea4e , but I want to see if our ci catches it.
@jakobbotschjakobbotschNov 9, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

It looks like we remove all virtual kinds below: 
runtime/src/coreclr/src/jit/morph.cpp

Lines 7917 to 7921
in
dc3101b
// This is now a direct call to the store args stub and not a tailcall.
 call->gtCallType = CT_USER_FUNC; 
 call->gtCallMethHnd = help.hStoreArgs; 
 call->gtFlags &= ~GTF_CALL_VIRT_KIND_MASK; 
 call->gtCallMoreFlags &= ~(GTF_CALL_M_TAILCALL | GTF_CALL_M_DELEGATE_INV | GTF_CALL_M_WRAPPER_DELEGATE_INV); 
I guess this line is not required but doesn't hurt since this is where we remove the arg for virtual stubs.
@sandreenkosandreenkoNov 10, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I see, thank you. I removed changes in fgMorphTailCallViaHelpers and fixed fgMorphTailCallViaJitHelper in the same way.
Sergey Andreenko
Don't wrap string literal const as nop for CoreRT.
bb0c5dc
It was probably an old workaround for another Jit bug, it is most likely fixed by now.
@sandreenkosandreenko
changed the title
Fix CoreRT const string handling.Change gtGetThisArg not to return nullptr.Nov 10, 2020
Sergey Andreenko
added 2 commits
November 10, 2020 10:58
Sergey Andreenko
Change gtGetThisArg not to return nullptr.
007f31f
There was only 1 case where a null return was tolerated - for a tail call in `optAssertionGen` marked as virtual.
However, a transformed tail call is never a virtual, fix `fgMorphTailCallViaJitHelper` to unset virtual flag.
Sergey Andreenko
Keep x86 tail call via jit helper as a virtual stub call.
8663da9
@sandreenko
sandreenko
commented
Nov 10, 2020

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Nov 10, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
sandreenko
sandreenko
commented
Nov 11, 2020
View reviewed changes
Comment threadsrc/coreclr/src/jit/morph.cpp
 thisPtr = objp;
 }

 // TODO-Cleanup: we leave it as a virtual stub call to
@sandreenkosandreenkoNov 11, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I made a try to clear GTF_CALL_VIRT_KIND_MASK  here but it caused failures because logic in LowerCall depends on this flag to be set in some cases, so I reverted that change.
It would be nice to clear it and fix the failures but it is out of the scope of this bug fix for CoreRT.
@runfoapprunfoappBot
mentioned this pull request
Nov 11, 2020

Closed

@sandreenko
sandreenko
commented
Nov 14, 2020

Copy link


Copy Markdown

ContributorAuthor
ping @dotnet/jit-contrib
@sandreenko
sandreenko
commented
Nov 19, 2020

Copy link


Copy Markdown

ContributorAuthor
Thanks @BruceForstall for the review!
@sandreenko
sandreenko
merged commit 044ee8c
into
dotnet:masterNov 19, 2020
@jkotasjkotas
mentioned this pull request
Nov 21, 2020
 Merged

@sandreenkosandreenko
mentioned this pull request
Nov 23, 2020
 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Dec 19, 2020
@sandreenko
sandreenko
deleted the

fixCoreRTConstStr

branch
May 17, 2021 07:50
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jakobbotschjakobbotschjakobbotsch left review comments

+1 more reviewer

@BruceForstallBruceForstallBruceForstall approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Assertion failed 'op1 != nullptr' in 'TestUtil.TestLog:VerifyOutput():int:this' during 'Morph - Global' (IL size 130)


3 participants

@sandreenko@jakobbotsch@BruceForstall

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Change gtGetThisArg not to return nullptr. - #44398
Merged
sandreenko merged 3 commits into
dotnet:masterfrom 
sandreenko:fixCoreRTConstStr
Nov 19, 2020
Merged
Change gtGetThisArg not to return nullptr.#44398
sandreenko merged 3 commits into
dotnet:masterfrom 
sandreenko:fixCoreRTConstStr
Conversation
@sandreenko
@sandreenkosandreenko
commented
Nov 9, 2020•

edited

Loading

Copy link


Copy Markdown

Contributor
bb0c5dc: Don't wrap string literal const as nop for CoreRT.
It was probably an old workaround for another Jit bug, it is most likely fixed by now.
2ae10db1383: Change gtGetThisArg not to return nullptr.
There was only 1 case where a null return was tolerated - for a tail call marked as virtual in optAssertionGen.

Check this case before we call gtGetThisArg.
Fixesdotnet/runtimelab#298
@sandreenkosandreenko
added
the 
area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
label
Nov 9, 2020
sandreenko
sandreenko
commented
Nov 9, 2020
View reviewed changes
Comment threadsrc/coreclr/src/jit/assertionprop.cpp
Outdated

@sandreenkosandreenkoNov 9, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

In fgMorphTailCallViaJitHelper we clear NeedsNullCheck, should not we also clear IsVirtual?

Then this condition will be just if (call->NeedsNullCheck() || call->IsVirtual())
@sandreenkosandreenkoNov 9, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

@jakobbotsch could you please advise me here?
@jakobbotschjakobbotschNov 9, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Sorry I don't know. fgMorphTailCallViaJitHelper is the old (from before #341) tailcall mechanism used only on x86. This mechanism might be used directly for virtual calls (or VSDs), but I don't remember how the address is computed in those cases.
@sandreenko
sandreenko
commented
Nov 9, 2020

Copy link


Copy Markdown

ContributorAuthor
PTAL @dotnet/jit-contrib
@sandreenko
sandreenko
commented
Nov 9, 2020

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Nov 9, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
sandreenko
sandreenko
commented
Nov 9, 2020
View reviewed changes
Comment threadsrc/coreclr/src/jit/morph.cpp
Outdated

@sandreenkosandreenkoNov 9, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

We do it for stub and in my understanding for vtable we should do the same, let's see what ci thinks.
@jakobbotschjakobbotschNov 9, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This is fgMorphTailCallViaHelpers which is for the new mechanism. Indeed those turn into normal direct calls always.
@sandreenkosandreenkoNov 9, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

it looks like I will need the same fix for fgMorphTailCallViaJitHelper: sandreenko@53cea4e , but I want to see if our ci catches it.
@jakobbotschjakobbotschNov 9, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

It looks like we remove all virtual kinds below: 
runtime/src/coreclr/src/jit/morph.cpp

Lines 7917 to 7921
in
dc3101b
// This is now a direct call to the store args stub and not a tailcall.
 call->gtCallType = CT_USER_FUNC; 
 call->gtCallMethHnd = help.hStoreArgs; 
 call->gtFlags &= ~GTF_CALL_VIRT_KIND_MASK; 
 call->gtCallMoreFlags &= ~(GTF_CALL_M_TAILCALL | GTF_CALL_M_DELEGATE_INV | GTF_CALL_M_WRAPPER_DELEGATE_INV); 
I guess this line is not required but doesn't hurt since this is where we remove the arg for virtual stubs.
@sandreenkosandreenkoNov 10, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I see, thank you. I removed changes in fgMorphTailCallViaHelpers and fixed fgMorphTailCallViaJitHelper in the same way.
Sergey Andreenko
Don't wrap string literal const as nop for CoreRT.
bb0c5dc
It was probably an old workaround for another Jit bug, it is most likely fixed by now.
@sandreenkosandreenko
changed the title
Fix CoreRT const string handling.Change gtGetThisArg not to return nullptr.Nov 10, 2020
Sergey Andreenko
added 2 commits
November 10, 2020 10:58
Sergey Andreenko
Change gtGetThisArg not to return nullptr.
007f31f
There was only 1 case where a null return was tolerated - for a tail call in `optAssertionGen` marked as virtual.
However, a transformed tail call is never a virtual, fix `fgMorphTailCallViaJitHelper` to unset virtual flag.
Sergey Andreenko
Keep x86 tail call via jit helper as a virtual stub call.
8663da9
@sandreenko
sandreenko
commented
Nov 10, 2020

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Nov 10, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
sandreenko
sandreenko
commented
Nov 11, 2020
View reviewed changes
Comment threadsrc/coreclr/src/jit/morph.cpp
 thisPtr = objp;
 }

 // TODO-Cleanup: we leave it as a virtual stub call to
@sandreenkosandreenkoNov 11, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I made a try to clear GTF_CALL_VIRT_KIND_MASK  here but it caused failures because logic in LowerCall depends on this flag to be set in some cases, so I reverted that change.
It would be nice to clear it and fix the failures but it is out of the scope of this bug fix for CoreRT.
@runfoapprunfoappBot
mentioned this pull request
Nov 11, 2020

Closed

@sandreenko
sandreenko
commented
Nov 14, 2020

Copy link


Copy Markdown

ContributorAuthor
ping @dotnet/jit-contrib
@sandreenko
sandreenko
commented
Nov 19, 2020

Copy link


Copy Markdown

ContributorAuthor
Thanks @BruceForstall for the review!
@sandreenko
sandreenko
merged commit 044ee8c
into
dotnet:masterNov 19, 2020
@jkotasjkotas
mentioned this pull request
Nov 21, 2020
 Merged

@sandreenkosandreenko
mentioned this pull request
Nov 23, 2020
 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Dec 19, 2020
@sandreenko
sandreenko
deleted the

fixCoreRTConstStr

branch
May 17, 2021 07:50
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jakobbotschjakobbotschjakobbotsch left review comments

+1 more reviewer

@BruceForstallBruceForstallBruceForstall approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Assertion failed 'op1 != nullptr' in 'TestUtil.TestLog:VerifyOutput():int:this' during 'Morph - Global' (IL size 130)


3 participants

@sandreenko@jakobbotsch@BruceForstall

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Change gtGetThisArg not to return nullptr. - #44398
Merged
sandreenko merged 3 commits into
dotnet:masterfrom 
sandreenko:fixCoreRTConstStr
Nov 19, 2020
Merged
Change gtGetThisArg not to return nullptr.#44398
sandreenko merged 3 commits into
dotnet:masterfrom 
sandreenko:fixCoreRTConstStr
Conversation
@sandreenko
@sandreenkosandreenko
commented
Nov 9, 2020•

edited

Loading

Copy link


Copy Markdown

Contributor
bb0c5dc: Don't wrap string literal const as nop for CoreRT.
It was probably an old workaround for another Jit bug, it is most likely fixed by now.
2ae10db1383: Change gtGetThisArg not to return nullptr.
There was only 1 case where a null return was tolerated - for a tail call marked as virtual in optAssertionGen.

Check this case before we call gtGetThisArg.
Fixesdotnet/runtimelab#298
@sandreenkosandreenko
added
the 
area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
label
Nov 9, 2020
sandreenko
sandreenko
commented
Nov 9, 2020
View reviewed changes
Comment threadsrc/coreclr/src/jit/assertionprop.cpp
Outdated

@sandreenkosandreenkoNov 9, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

In fgMorphTailCallViaJitHelper we clear NeedsNullCheck, should not we also clear IsVirtual?

Then this condition will be just if (call->NeedsNullCheck() || call->IsVirtual())
@sandreenkosandreenkoNov 9, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

@jakobbotsch could you please advise me here?
@jakobbotschjakobbotschNov 9, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Sorry I don't know. fgMorphTailCallViaJitHelper is the old (from before #341) tailcall mechanism used only on x86. This mechanism might be used directly for virtual calls (or VSDs), but I don't remember how the address is computed in those cases.
@sandreenko
sandreenko
commented
Nov 9, 2020

Copy link


Copy Markdown

ContributorAuthor
PTAL @dotnet/jit-contrib
@sandreenko
sandreenko
commented
Nov 9, 2020

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Nov 9, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
sandreenko
sandreenko
commented
Nov 9, 2020
View reviewed changes
Comment threadsrc/coreclr/src/jit/morph.cpp
Outdated

@sandreenkosandreenkoNov 9, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

We do it for stub and in my understanding for vtable we should do the same, let's see what ci thinks.
@jakobbotschjakobbotschNov 9, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This is fgMorphTailCallViaHelpers which is for the new mechanism. Indeed those turn into normal direct calls always.
@sandreenkosandreenkoNov 9, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

it looks like I will need the same fix for fgMorphTailCallViaJitHelper: sandreenko@53cea4e , but I want to see if our ci catches it.
@jakobbotschjakobbotschNov 9, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

It looks like we remove all virtual kinds below: 
runtime/src/coreclr/src/jit/morph.cpp

Lines 7917 to 7921
in
dc3101b
// This is now a direct call to the store args stub and not a tailcall.
 call->gtCallType = CT_USER_FUNC; 
 call->gtCallMethHnd = help.hStoreArgs; 
 call->gtFlags &= ~GTF_CALL_VIRT_KIND_MASK; 
 call->gtCallMoreFlags &= ~(GTF_CALL_M_TAILCALL | GTF_CALL_M_DELEGATE_INV | GTF_CALL_M_WRAPPER_DELEGATE_INV); 
I guess this line is not required but doesn't hurt since this is where we remove the arg for virtual stubs.
@sandreenkosandreenkoNov 10, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I see, thank you. I removed changes in fgMorphTailCallViaHelpers and fixed fgMorphTailCallViaJitHelper in the same way.
Sergey Andreenko
Don't wrap string literal const as nop for CoreRT.
bb0c5dc
It was probably an old workaround for another Jit bug, it is most likely fixed by now.
@sandreenkosandreenko
changed the title
Fix CoreRT const string handling.Change gtGetThisArg not to return nullptr.Nov 10, 2020
Sergey Andreenko
added 2 commits
November 10, 2020 10:58
Sergey Andreenko
Change gtGetThisArg not to return nullptr.
007f31f
There was only 1 case where a null return was tolerated - for a tail call in `optAssertionGen` marked as virtual.
However, a transformed tail call is never a virtual, fix `fgMorphTailCallViaJitHelper` to unset virtual flag.
Sergey Andreenko
Keep x86 tail call via jit helper as a virtual stub call.
8663da9
@sandreenko
sandreenko
commented
Nov 10, 2020

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Nov 10, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
sandreenko
sandreenko
commented
Nov 11, 2020
View reviewed changes
Comment threadsrc/coreclr/src/jit/morph.cpp
 thisPtr = objp;
 }

 // TODO-Cleanup: we leave it as a virtual stub call to
@sandreenkosandreenkoNov 11, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I made a try to clear GTF_CALL_VIRT_KIND_MASK  here but it caused failures because logic in LowerCall depends on this flag to be set in some cases, so I reverted that change.
It would be nice to clear it and fix the failures but it is out of the scope of this bug fix for CoreRT.
@runfoapprunfoappBot
mentioned this pull request
Nov 11, 2020

Closed

@sandreenko
sandreenko
commented
Nov 14, 2020

Copy link


Copy Markdown

ContributorAuthor
ping @dotnet/jit-contrib
@sandreenko
sandreenko
commented
Nov 19, 2020

Copy link


Copy Markdown

ContributorAuthor
Thanks @BruceForstall for the review!
@sandreenko
sandreenko
merged commit 044ee8c
into
dotnet:masterNov 19, 2020
@jkotasjkotas
mentioned this pull request
Nov 21, 2020
 Merged

@sandreenkosandreenko
mentioned this pull request
Nov 23, 2020
 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Dec 19, 2020
@sandreenko
sandreenko
deleted the

fixCoreRTConstStr

branch
May 17, 2021 07:50
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jakobbotschjakobbotschjakobbotsch left review comments

+1 more reviewer

@BruceForstallBruceForstallBruceForstall approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Assertion failed 'op1 != nullptr' in 'TestUtil.TestLog:VerifyOutput():int:this' during 'Morph - Global' (IL size 130)


3 participants

@sandreenko@jakobbotsch@BruceForstall

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
Change gtGetThisArg not to return nullptr. - #44398
Merged
sandreenko merged 3 commits into
dotnet:masterfrom 
sandreenko:fixCoreRTConstStr
Nov 19, 2020
Merged
Change gtGetThisArg not to return nullptr.#44398
sandreenko merged 3 commits into
dotnet:masterfrom 
sandreenko:fixCoreRTConstStr
Conversation
@sandreenko
@sandreenkosandreenko
commented
Nov 9, 2020•

edited

Loading

Copy link


Copy Markdown

Contributor
bb0c5dc: Don't wrap string literal const as nop for CoreRT.
It was probably an old workaround for another Jit bug, it is most likely fixed by now.
2ae10db1383: Change gtGetThisArg not to return nullptr.
There was only 1 case where a null return was tolerated - for a tail call marked as virtual in optAssertionGen.

Check this case before we call gtGetThisArg.
Fixesdotnet/runtimelab#298
@sandreenkosandreenko
added
the 
area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
label
Nov 9, 2020
sandreenko
sandreenko
commented
Nov 9, 2020
View reviewed changes
Comment threadsrc/coreclr/src/jit/assertionprop.cpp
Outdated

@sandreenkosandreenkoNov 9, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

In fgMorphTailCallViaJitHelper we clear NeedsNullCheck, should not we also clear IsVirtual?

Then this condition will be just if (call->NeedsNullCheck() || call->IsVirtual())
@sandreenkosandreenkoNov 9, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

@jakobbotsch could you please advise me here?
@jakobbotschjakobbotschNov 9, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Sorry I don't know. fgMorphTailCallViaJitHelper is the old (from before #341) tailcall mechanism used only on x86. This mechanism might be used directly for virtual calls (or VSDs), but I don't remember how the address is computed in those cases.
@sandreenko
sandreenko
commented
Nov 9, 2020

Copy link


Copy Markdown

ContributorAuthor
PTAL @dotnet/jit-contrib
@sandreenko
sandreenko
commented
Nov 9, 2020

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Nov 9, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
sandreenko
sandreenko
commented
Nov 9, 2020
View reviewed changes
Comment threadsrc/coreclr/src/jit/morph.cpp
Outdated

@sandreenkosandreenkoNov 9, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

We do it for stub and in my understanding for vtable we should do the same, let's see what ci thinks.
@jakobbotschjakobbotschNov 9, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This is fgMorphTailCallViaHelpers which is for the new mechanism. Indeed those turn into normal direct calls always.
@sandreenkosandreenkoNov 9, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

it looks like I will need the same fix for fgMorphTailCallViaJitHelper: sandreenko@53cea4e , but I want to see if our ci catches it.
@jakobbotschjakobbotschNov 9, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

It looks like we remove all virtual kinds below: 
runtime/src/coreclr/src/jit/morph.cpp

Lines 7917 to 7921
in
dc3101b
// This is now a direct call to the store args stub and not a tailcall.
 call->gtCallType = CT_USER_FUNC; 
 call->gtCallMethHnd = help.hStoreArgs; 
 call->gtFlags &= ~GTF_CALL_VIRT_KIND_MASK; 
 call->gtCallMoreFlags &= ~(GTF_CALL_M_TAILCALL | GTF_CALL_M_DELEGATE_INV | GTF_CALL_M_WRAPPER_DELEGATE_INV); 
I guess this line is not required but doesn't hurt since this is where we remove the arg for virtual stubs.
@sandreenkosandreenkoNov 10, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I see, thank you. I removed changes in fgMorphTailCallViaHelpers and fixed fgMorphTailCallViaJitHelper in the same way.
Sergey Andreenko
Don't wrap string literal const as nop for CoreRT.
bb0c5dc
It was probably an old workaround for another Jit bug, it is most likely fixed by now.
@sandreenkosandreenko
changed the title
Fix CoreRT const string handling.Change gtGetThisArg not to return nullptr.Nov 10, 2020
Sergey Andreenko
added 2 commits
November 10, 2020 10:58
Sergey Andreenko
Change gtGetThisArg not to return nullptr.
007f31f
There was only 1 case where a null return was tolerated - for a tail call in `optAssertionGen` marked as virtual.
However, a transformed tail call is never a virtual, fix `fgMorphTailCallViaJitHelper` to unset virtual flag.
Sergey Andreenko
Keep x86 tail call via jit helper as a virtual stub call.
8663da9
@sandreenko
sandreenko
commented
Nov 10, 2020

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Nov 10, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
sandreenko
sandreenko
commented
Nov 11, 2020
View reviewed changes
Comment threadsrc/coreclr/src/jit/morph.cpp
 thisPtr = objp;
 }

 // TODO-Cleanup: we leave it as a virtual stub call to
@sandreenkosandreenkoNov 11, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I made a try to clear GTF_CALL_VIRT_KIND_MASK  here but it caused failures because logic in LowerCall depends on this flag to be set in some cases, so I reverted that change.
It would be nice to clear it and fix the failures but it is out of the scope of this bug fix for CoreRT.
@runfoapprunfoappBot
mentioned this pull request
Nov 11, 2020

Closed

@sandreenko
sandreenko
commented
Nov 14, 2020

Copy link


Copy Markdown

ContributorAuthor
ping @dotnet/jit-contrib
@sandreenko
sandreenko
commented
Nov 19, 2020

Copy link


Copy Markdown

ContributorAuthor
Thanks @BruceForstall for the review!
@sandreenko
sandreenko
merged commit 044ee8c
into
dotnet:masterNov 19, 2020
@jkotasjkotas
mentioned this pull request
Nov 21, 2020
 Merged

@sandreenkosandreenko
mentioned this pull request
Nov 23, 2020
 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Dec 19, 2020
@sandreenko
sandreenko
deleted the

fixCoreRTConstStr

branch
May 17, 2021 07:50
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jakobbotschjakobbotschjakobbotsch left review comments

+1 more reviewer

@BruceForstallBruceForstallBruceForstall approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Assertion failed 'op1 != nullptr' in 'TestUtil.TestLog:VerifyOutput():int:this' during 'Morph - Global' (IL size 130)


3 participants

@sandreenko@jakobbotsch@BruceForstall

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Change gtGetThisArg not to return nullptr. - #44398
Merged
sandreenko merged 3 commits into
dotnet:masterfrom 
sandreenko:fixCoreRTConstStr
Nov 19, 2020
Merged
Change gtGetThisArg not to return nullptr.#44398
sandreenko merged 3 commits into
dotnet:masterfrom 
sandreenko:fixCoreRTConstStr
Conversation
@sandreenko
@sandreenkosandreenko
commented
Nov 9, 2020•

edited

Loading

Copy link


Copy Markdown

Contributor
bb0c5dc: Don't wrap string literal const as nop for CoreRT.
It was probably an old workaround for another Jit bug, it is most likely fixed by now.
2ae10db1383: Change gtGetThisArg not to return nullptr.
There was only 1 case where a null return was tolerated - for a tail call marked as virtual in optAssertionGen.

Check this case before we call gtGetThisArg.
Fixesdotnet/runtimelab#298
@sandreenkosandreenko
added
the 
area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
label
Nov 9, 2020
sandreenko
sandreenko
commented
Nov 9, 2020
View reviewed changes
Comment threadsrc/coreclr/src/jit/assertionprop.cpp
Outdated

@sandreenkosandreenkoNov 9, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

In fgMorphTailCallViaJitHelper we clear NeedsNullCheck, should not we also clear IsVirtual?

Then this condition will be just if (call->NeedsNullCheck() || call->IsVirtual())
@sandreenkosandreenkoNov 9, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

@jakobbotsch could you please advise me here?
@jakobbotschjakobbotschNov 9, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Sorry I don't know. fgMorphTailCallViaJitHelper is the old (from before #341) tailcall mechanism used only on x86. This mechanism might be used directly for virtual calls (or VSDs), but I don't remember how the address is computed in those cases.
@sandreenko
sandreenko
commented
Nov 9, 2020

Copy link


Copy Markdown

ContributorAuthor
PTAL @dotnet/jit-contrib
@sandreenko
sandreenko
commented
Nov 9, 2020

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Nov 9, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
sandreenko
sandreenko
commented
Nov 9, 2020
View reviewed changes
Comment threadsrc/coreclr/src/jit/morph.cpp
Outdated

@sandreenkosandreenkoNov 9, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

We do it for stub and in my understanding for vtable we should do the same, let's see what ci thinks.
@jakobbotschjakobbotschNov 9, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This is fgMorphTailCallViaHelpers which is for the new mechanism. Indeed those turn into normal direct calls always.
@sandreenkosandreenkoNov 9, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

it looks like I will need the same fix for fgMorphTailCallViaJitHelper: sandreenko@53cea4e , but I want to see if our ci catches it.
@jakobbotschjakobbotschNov 9, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

It looks like we remove all virtual kinds below: 
runtime/src/coreclr/src/jit/morph.cpp

Lines 7917 to 7921
in
dc3101b
// This is now a direct call to the store args stub and not a tailcall.
 call->gtCallType = CT_USER_FUNC; 
 call->gtCallMethHnd = help.hStoreArgs; 
 call->gtFlags &= ~GTF_CALL_VIRT_KIND_MASK; 
 call->gtCallMoreFlags &= ~(GTF_CALL_M_TAILCALL | GTF_CALL_M_DELEGATE_INV | GTF_CALL_M_WRAPPER_DELEGATE_INV); 
I guess this line is not required but doesn't hurt since this is where we remove the arg for virtual stubs.
@sandreenkosandreenkoNov 10, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I see, thank you. I removed changes in fgMorphTailCallViaHelpers and fixed fgMorphTailCallViaJitHelper in the same way.
Sergey Andreenko
Don't wrap string literal const as nop for CoreRT.
bb0c5dc
It was probably an old workaround for another Jit bug, it is most likely fixed by now.
@sandreenkosandreenko
changed the title
Fix CoreRT const string handling.Change gtGetThisArg not to return nullptr.Nov 10, 2020
Sergey Andreenko
added 2 commits
November 10, 2020 10:58
Sergey Andreenko
Change gtGetThisArg not to return nullptr.
007f31f
There was only 1 case where a null return was tolerated - for a tail call in `optAssertionGen` marked as virtual.
However, a transformed tail call is never a virtual, fix `fgMorphTailCallViaJitHelper` to unset virtual flag.
Sergey Andreenko
Keep x86 tail call via jit helper as a virtual stub call.
8663da9
@sandreenko
sandreenko
commented
Nov 10, 2020

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Nov 10, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
sandreenko
sandreenko
commented
Nov 11, 2020
View reviewed changes
Comment threadsrc/coreclr/src/jit/morph.cpp
 thisPtr = objp;
 }

 // TODO-Cleanup: we leave it as a virtual stub call to
@sandreenkosandreenkoNov 11, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I made a try to clear GTF_CALL_VIRT_KIND_MASK  here but it caused failures because logic in LowerCall depends on this flag to be set in some cases, so I reverted that change.
It would be nice to clear it and fix the failures but it is out of the scope of this bug fix for CoreRT.
@runfoapprunfoappBot
mentioned this pull request
Nov 11, 2020

Closed

@sandreenko
sandreenko
commented
Nov 14, 2020

Copy link


Copy Markdown

ContributorAuthor
ping @dotnet/jit-contrib
@sandreenko
sandreenko
commented
Nov 19, 2020

Copy link


Copy Markdown

ContributorAuthor
Thanks @BruceForstall for the review!
@sandreenko
sandreenko
merged commit 044ee8c
into
dotnet:masterNov 19, 2020
@jkotasjkotas
mentioned this pull request
Nov 21, 2020
 Merged

@sandreenkosandreenko
mentioned this pull request
Nov 23, 2020
 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Dec 19, 2020
@sandreenko
sandreenko
deleted the

fixCoreRTConstStr

branch
May 17, 2021 07:50
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jakobbotschjakobbotschjakobbotsch left review comments

+1 more reviewer

@BruceForstallBruceForstallBruceForstall approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Assertion failed 'op1 != nullptr' in 'TestUtil.TestLog:VerifyOutput():int:this' during 'Morph - Global' (IL size 130)


3 participants

@sandreenko@jakobbotsch@BruceForstall

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Change gtGetThisArg not to return nullptr. - #44398
Merged
sandreenko merged 3 commits into
dotnet:masterfrom 
sandreenko:fixCoreRTConstStr
Nov 19, 2020
Merged
Change gtGetThisArg not to return nullptr.#44398
sandreenko merged 3 commits into
dotnet:masterfrom 
sandreenko:fixCoreRTConstStr
Conversation
@sandreenko
@sandreenkosandreenko
commented
Nov 9, 2020•

edited

Loading

Copy link


Copy Markdown

Contributor
bb0c5dc: Don't wrap string literal const as nop for CoreRT.
It was probably an old workaround for another Jit bug, it is most likely fixed by now.
2ae10db1383: Change gtGetThisArg not to return nullptr.
There was only 1 case where a null return was tolerated - for a tail call marked as virtual in optAssertionGen.

Check this case before we call gtGetThisArg.
Fixesdotnet/runtimelab#298
@sandreenkosandreenko
added
the 
area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
label
Nov 9, 2020
sandreenko
sandreenko
commented
Nov 9, 2020
View reviewed changes
Comment threadsrc/coreclr/src/jit/assertionprop.cpp
Outdated

@sandreenkosandreenkoNov 9, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

In fgMorphTailCallViaJitHelper we clear NeedsNullCheck, should not we also clear IsVirtual?

Then this condition will be just if (call->NeedsNullCheck() || call->IsVirtual())
@sandreenkosandreenkoNov 9, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

@jakobbotsch could you please advise me here?
@jakobbotschjakobbotschNov 9, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Sorry I don't know. fgMorphTailCallViaJitHelper is the old (from before #341) tailcall mechanism used only on x86. This mechanism might be used directly for virtual calls (or VSDs), but I don't remember how the address is computed in those cases.
@sandreenko
sandreenko
commented
Nov 9, 2020

Copy link


Copy Markdown

ContributorAuthor
PTAL @dotnet/jit-contrib
@sandreenko
sandreenko
commented
Nov 9, 2020

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Nov 9, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
sandreenko
sandreenko
commented
Nov 9, 2020
View reviewed changes
Comment threadsrc/coreclr/src/jit/morph.cpp
Outdated

@sandreenkosandreenkoNov 9, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

We do it for stub and in my understanding for vtable we should do the same, let's see what ci thinks.
@jakobbotschjakobbotschNov 9, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This is fgMorphTailCallViaHelpers which is for the new mechanism. Indeed those turn into normal direct calls always.
@sandreenkosandreenkoNov 9, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

it looks like I will need the same fix for fgMorphTailCallViaJitHelper: sandreenko@53cea4e , but I want to see if our ci catches it.
@jakobbotschjakobbotschNov 9, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

It looks like we remove all virtual kinds below: 
runtime/src/coreclr/src/jit/morph.cpp

Lines 7917 to 7921
in
dc3101b
// This is now a direct call to the store args stub and not a tailcall.
 call->gtCallType = CT_USER_FUNC; 
 call->gtCallMethHnd = help.hStoreArgs; 
 call->gtFlags &= ~GTF_CALL_VIRT_KIND_MASK; 
 call->gtCallMoreFlags &= ~(GTF_CALL_M_TAILCALL | GTF_CALL_M_DELEGATE_INV | GTF_CALL_M_WRAPPER_DELEGATE_INV); 
I guess this line is not required but doesn't hurt since this is where we remove the arg for virtual stubs.
@sandreenkosandreenkoNov 10, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I see, thank you. I removed changes in fgMorphTailCallViaHelpers and fixed fgMorphTailCallViaJitHelper in the same way.
Sergey Andreenko
Don't wrap string literal const as nop for CoreRT.
bb0c5dc
It was probably an old workaround for another Jit bug, it is most likely fixed by now.
@sandreenkosandreenko
changed the title
Fix CoreRT const string handling.Change gtGetThisArg not to return nullptr.Nov 10, 2020
Sergey Andreenko
added 2 commits
November 10, 2020 10:58
Sergey Andreenko
Change gtGetThisArg not to return nullptr.
007f31f
There was only 1 case where a null return was tolerated - for a tail call in `optAssertionGen` marked as virtual.
However, a transformed tail call is never a virtual, fix `fgMorphTailCallViaJitHelper` to unset virtual flag.
Sergey Andreenko
Keep x86 tail call via jit helper as a virtual stub call.
8663da9
@sandreenko
sandreenko
commented
Nov 10, 2020

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Nov 10, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
sandreenko
sandreenko
commented
Nov 11, 2020
View reviewed changes
Comment threadsrc/coreclr/src/jit/morph.cpp
 thisPtr = objp;
 }

 // TODO-Cleanup: we leave it as a virtual stub call to
@sandreenkosandreenkoNov 11, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I made a try to clear GTF_CALL_VIRT_KIND_MASK  here but it caused failures because logic in LowerCall depends on this flag to be set in some cases, so I reverted that change.
It would be nice to clear it and fix the failures but it is out of the scope of this bug fix for CoreRT.
@runfoapprunfoappBot
mentioned this pull request
Nov 11, 2020

Closed

@sandreenko
sandreenko
commented
Nov 14, 2020

Copy link


Copy Markdown

ContributorAuthor
ping @dotnet/jit-contrib
@sandreenko
sandreenko
commented
Nov 19, 2020

Copy link


Copy Markdown

ContributorAuthor
Thanks @BruceForstall for the review!
@sandreenko
sandreenko
merged commit 044ee8c
into
dotnet:masterNov 19, 2020
@jkotasjkotas
mentioned this pull request
Nov 21, 2020
 Merged

@sandreenkosandreenko
mentioned this pull request
Nov 23, 2020
 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Dec 19, 2020
@sandreenko
sandreenko
deleted the

fixCoreRTConstStr

branch
May 17, 2021 07:50
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jakobbotschjakobbotschjakobbotsch left review comments

+1 more reviewer

@BruceForstallBruceForstallBruceForstall approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Assertion failed 'op1 != nullptr' in 'TestUtil.TestLog:VerifyOutput():int:this' during 'Morph - Global' (IL size 130)


3 participants

@sandreenko@jakobbotsch@BruceForstall

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
Change gtGetThisArg not to return nullptr. - #44398
Merged
sandreenko merged 3 commits into
dotnet:masterfrom 
sandreenko:fixCoreRTConstStr
Nov 19, 2020
Merged
Change gtGetThisArg not to return nullptr.#44398
sandreenko merged 3 commits into
dotnet:masterfrom 
sandreenko:fixCoreRTConstStr
Conversation
@sandreenko
@sandreenkosandreenko
commented
Nov 9, 2020•

edited

Loading

Copy link


Copy Markdown

Contributor
bb0c5dc: Don't wrap string literal const as nop for CoreRT.
It was probably an old workaround for another Jit bug, it is most likely fixed by now.
2ae10db1383: Change gtGetThisArg not to return nullptr.
There was only 1 case where a null return was tolerated - for a tail call marked as virtual in optAssertionGen.

Check this case before we call gtGetThisArg.
Fixesdotnet/runtimelab#298
@sandreenkosandreenko
added
the 
area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
label
Nov 9, 2020
sandreenko
sandreenko
commented
Nov 9, 2020
View reviewed changes
Comment threadsrc/coreclr/src/jit/assertionprop.cpp
Outdated

@sandreenkosandreenkoNov 9, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

In fgMorphTailCallViaJitHelper we clear NeedsNullCheck, should not we also clear IsVirtual?

Then this condition will be just if (call->NeedsNullCheck() || call->IsVirtual())
@sandreenkosandreenkoNov 9, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

@jakobbotsch could you please advise me here?
@jakobbotschjakobbotschNov 9, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Sorry I don't know. fgMorphTailCallViaJitHelper is the old (from before #341) tailcall mechanism used only on x86. This mechanism might be used directly for virtual calls (or VSDs), but I don't remember how the address is computed in those cases.
@sandreenko
sandreenko
commented
Nov 9, 2020

Copy link


Copy Markdown

ContributorAuthor
PTAL @dotnet/jit-contrib
@sandreenko
sandreenko
commented
Nov 9, 2020

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Nov 9, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
sandreenko
sandreenko
commented
Nov 9, 2020
View reviewed changes
Comment threadsrc/coreclr/src/jit/morph.cpp
Outdated

@sandreenkosandreenkoNov 9, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

We do it for stub and in my understanding for vtable we should do the same, let's see what ci thinks.
@jakobbotschjakobbotschNov 9, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This is fgMorphTailCallViaHelpers which is for the new mechanism. Indeed those turn into normal direct calls always.
@sandreenkosandreenkoNov 9, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

it looks like I will need the same fix for fgMorphTailCallViaJitHelper: sandreenko@53cea4e , but I want to see if our ci catches it.
@jakobbotschjakobbotschNov 9, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

It looks like we remove all virtual kinds below: 
runtime/src/coreclr/src/jit/morph.cpp

Lines 7917 to 7921
in
dc3101b
// This is now a direct call to the store args stub and not a tailcall.
 call->gtCallType = CT_USER_FUNC; 
 call->gtCallMethHnd = help.hStoreArgs; 
 call->gtFlags &= ~GTF_CALL_VIRT_KIND_MASK; 
 call->gtCallMoreFlags &= ~(GTF_CALL_M_TAILCALL | GTF_CALL_M_DELEGATE_INV | GTF_CALL_M_WRAPPER_DELEGATE_INV); 
I guess this line is not required but doesn't hurt since this is where we remove the arg for virtual stubs.
@sandreenkosandreenkoNov 10, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I see, thank you. I removed changes in fgMorphTailCallViaHelpers and fixed fgMorphTailCallViaJitHelper in the same way.
Sergey Andreenko
Don't wrap string literal const as nop for CoreRT.
bb0c5dc
It was probably an old workaround for another Jit bug, it is most likely fixed by now.
@sandreenkosandreenko
changed the title
Fix CoreRT const string handling.Change gtGetThisArg not to return nullptr.Nov 10, 2020
Sergey Andreenko
added 2 commits
November 10, 2020 10:58
Sergey Andreenko
Change gtGetThisArg not to return nullptr.
007f31f
There was only 1 case where a null return was tolerated - for a tail call in `optAssertionGen` marked as virtual.
However, a transformed tail call is never a virtual, fix `fgMorphTailCallViaJitHelper` to unset virtual flag.
Sergey Andreenko
Keep x86 tail call via jit helper as a virtual stub call.
8663da9
@sandreenko
sandreenko
commented
Nov 10, 2020

Copy link


Copy Markdown

ContributorAuthor
/azp run runtime-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Nov 10, 2020

Copy link


Copy Markdown


Azure Pipelines successfully started running 1 pipeline(s).
sandreenko
sandreenko
commented
Nov 11, 2020
View reviewed changes
Comment threadsrc/coreclr/src/jit/morph.cpp
 thisPtr = objp;
 }

 // TODO-Cleanup: we leave it as a virtual stub call to
@sandreenkosandreenkoNov 11, 2020

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I made a try to clear GTF_CALL_VIRT_KIND_MASK  here but it caused failures because logic in LowerCall depends on this flag to be set in some cases, so I reverted that change.
It would be nice to clear it and fix the failures but it is out of the scope of this bug fix for CoreRT.
@runfoapprunfoappBot
mentioned this pull request
Nov 11, 2020

Closed

@sandreenko
sandreenko
commented
Nov 14, 2020

Copy link


Copy Markdown

ContributorAuthor
ping @dotnet/jit-contrib
@sandreenko
sandreenko
commented
Nov 19, 2020

Copy link


Copy Markdown

ContributorAuthor
Thanks @BruceForstall for the review!
@sandreenko
sandreenko
merged commit 044ee8c
into
dotnet:masterNov 19, 2020
@jkotasjkotas
mentioned this pull request
Nov 21, 2020
 Merged

@sandreenkosandreenko
mentioned this pull request
Nov 23, 2020
 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Dec 19, 2020
@sandreenko
sandreenko
deleted the

fixCoreRTConstStr

branch
May 17, 2021 07:50
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jakobbotschjakobbotschjakobbotsch left review comments

+1 more reviewer

@BruceForstallBruceForstallBruceForstall approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

Assertion failed 'op1 != nullptr' in 'TestUtil.TestLog:VerifyOutput():int:this' during 'Morph - Global' (IL size 130)


3 participants

@sandreenko@jakobbotsch@BruceForstall