Do not embed result type in VN for scalar hw intrinsics - #72895

Merged
kunalspathak merged 3 commits into
dotnet:mainfrom
kunalspathak:dumpsimdType
Jul 28, 2022
Merged

Do not embed result type in VN for scalar hw intrinsics#72895
kunalspathak merged 3 commits into
dotnet:mainfrom
kunalspathak:dumpsimdType

Conversation

@kunalspathak

Copy link
Copy Markdown
Contributor

For scalar hw intrinsics, we were creating VNForSimdType and it was manifesting in vnDumpSimdType().

Fixes: #69551

@ghostghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jul 26, 2022
@kunalspathak

Copy link
Copy Markdown
ContributorAuthor

@SingleAccretion , @dotnet/jit-contrib , @tannergooding

@ghost

Copy link
Copy Markdown

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

Issue Details

For scalar hw intrinsics, we were creating VNForSimdType and it was manifesting in vnDumpSimdType().

Fixes: #69551

Author:kunalspathak
Assignees:-
Labels:

area-CodeGen-coreclr

Milestone:-

@kunalspathakkunalspathak changed the title Do not embed result type for scalar hw intrinsicsDo not embed result type in VN for scalar hw intrinsicsJul 26, 2022
Comment threadsrc/coreclr/jit/hwintrinsic.cpp Outdated
Comment on lines +215 to +218
if (HWIntrinsicInfo::lookupCategory(hwIntrinsicID) == HW_Category_Scalar)
{
return false;
}

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.

We may also want a path for HW_Category_Special or maybe better check for:

Suggested change
if (HWIntrinsicInfo::lookupCategory(hwIntrinsicID) == HW_Category_Scalar)
{
returnfalse;
}
unsigned simdSize = 0;
if (HWIntrinsicInfo::tryLookupSimdSize(hwIntrinsicID, &simdSize) && (simdSize == 0))
{
returnfalse;
}

All scalar intrinsics, regardless of category, should have a simdSize == 0. Likewise, all SIMD intrinsics should have a size of 8, 16, 32, or -1 (where -1 implies the intrinsic supports multiple SIMD sizes and is not a scalar).

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.

I think the current check is "accurate enough" today since the few "special" scalars tend to have all INS_invalid entries and are specially handled in codegen instead, but its probably better to be more explicit here.

There's also a consideration for the type encoding that certain special or helper intrinsics might specify INS_invalid and get specialized handling in lowering/codegen and so it might be important to have the type anyways. I'm unsure if there is any actual issue here or not, but its probably worth auditing at some point in the future.

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 agree, except that it doesn't set the simdSize if it was -1. So probably I should initialize simdSize to something like 100 or something and then see if became 0. Wil fix it.

@tannergoodingtannergoodingJul 28, 2022

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.

That's what the tryLookupSimdSize check was for. It should return false if simdSize == -1 and so the second check (simdSize == 0) won't get hit and we'll treat it as SIMD rather than Scalar

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.

ah. was an oversight on my part.

@kunalspathak
kunalspathak merged commit ef7c980 into dotnet:mainJul 28, 2022
@kunalspathak
kunalspathak deleted the dumpsimdType branch July 28, 2022 20:21
@ghostghost locked as resolved and limited conversation to collaborators Aug 28, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-CodeGen-coreclrCLR 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 'preciseVarTypeMap[type] != TYP_UNDEF' when dumping all functions

2 participants

@kunalspathak@tannergooding
, '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

Do not embed result type in VN for scalar hw intrinsics - #72895

Merged
kunalspathak merged 3 commits into
dotnet:mainfrom
kunalspathak:dumpsimdType
Jul 28, 2022
Merged

Do not embed result type in VN for scalar hw intrinsics#72895
kunalspathak merged 3 commits into
dotnet:mainfrom
kunalspathak:dumpsimdType

Conversation

@kunalspathak

Copy link
Copy Markdown
Contributor

For scalar hw intrinsics, we were creating VNForSimdType and it was manifesting in vnDumpSimdType().

Fixes: #69551

@ghostghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jul 26, 2022
@kunalspathak

Copy link
Copy Markdown
ContributorAuthor

@SingleAccretion , @dotnet/jit-contrib , @tannergooding

@ghost

Copy link
Copy Markdown

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

Issue Details

For scalar hw intrinsics, we were creating VNForSimdType and it was manifesting in vnDumpSimdType().

Fixes: #69551

Author:kunalspathak
Assignees:-
Labels:

area-CodeGen-coreclr

Milestone:-

@kunalspathakkunalspathak changed the title Do not embed result type for scalar hw intrinsicsDo not embed result type in VN for scalar hw intrinsicsJul 26, 2022
Comment threadsrc/coreclr/jit/hwintrinsic.cpp Outdated
Comment on lines +215 to +218
if (HWIntrinsicInfo::lookupCategory(hwIntrinsicID) == HW_Category_Scalar)
{
return false;
}

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.

We may also want a path for HW_Category_Special or maybe better check for:

Suggested change
if (HWIntrinsicInfo::lookupCategory(hwIntrinsicID) == HW_Category_Scalar)
{
returnfalse;
}
unsigned simdSize = 0;
if (HWIntrinsicInfo::tryLookupSimdSize(hwIntrinsicID, &simdSize) && (simdSize == 0))
{
returnfalse;
}

All scalar intrinsics, regardless of category, should have a simdSize == 0. Likewise, all SIMD intrinsics should have a size of 8, 16, 32, or -1 (where -1 implies the intrinsic supports multiple SIMD sizes and is not a scalar).

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.

I think the current check is "accurate enough" today since the few "special" scalars tend to have all INS_invalid entries and are specially handled in codegen instead, but its probably better to be more explicit here.

There's also a consideration for the type encoding that certain special or helper intrinsics might specify INS_invalid and get specialized handling in lowering/codegen and so it might be important to have the type anyways. I'm unsure if there is any actual issue here or not, but its probably worth auditing at some point in the future.

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 agree, except that it doesn't set the simdSize if it was -1. So probably I should initialize simdSize to something like 100 or something and then see if became 0. Wil fix it.

@tannergoodingtannergoodingJul 28, 2022

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.

That's what the tryLookupSimdSize check was for. It should return false if simdSize == -1 and so the second check (simdSize == 0) won't get hit and we'll treat it as SIMD rather than Scalar

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.

ah. was an oversight on my part.

@kunalspathak
kunalspathak merged commit ef7c980 into dotnet:mainJul 28, 2022
@kunalspathak
kunalspathak deleted the dumpsimdType branch July 28, 2022 20:21
@ghostghost locked as resolved and limited conversation to collaborators Aug 28, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-CodeGen-coreclrCLR 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 'preciseVarTypeMap[type] != TYP_UNDEF' when dumping all functions

2 participants

@kunalspathak@tannergooding
, '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

Do not embed result type in VN for scalar hw intrinsics - #72895

Merged
kunalspathak merged 3 commits into
dotnet:mainfrom
kunalspathak:dumpsimdType
Jul 28, 2022
Merged

Do not embed result type in VN for scalar hw intrinsics#72895
kunalspathak merged 3 commits into
dotnet:mainfrom
kunalspathak:dumpsimdType

Conversation

@kunalspathak

Copy link
Copy Markdown
Contributor

For scalar hw intrinsics, we were creating VNForSimdType and it was manifesting in vnDumpSimdType().

Fixes: #69551

@ghostghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jul 26, 2022
@kunalspathak

Copy link
Copy Markdown
ContributorAuthor

@SingleAccretion , @dotnet/jit-contrib , @tannergooding

@ghost

Copy link
Copy Markdown

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

Issue Details

For scalar hw intrinsics, we were creating VNForSimdType and it was manifesting in vnDumpSimdType().

Fixes: #69551

Author:kunalspathak
Assignees:-
Labels:

area-CodeGen-coreclr

Milestone:-

@kunalspathakkunalspathak changed the title Do not embed result type for scalar hw intrinsicsDo not embed result type in VN for scalar hw intrinsicsJul 26, 2022
Comment threadsrc/coreclr/jit/hwintrinsic.cpp Outdated
Comment on lines +215 to +218
if (HWIntrinsicInfo::lookupCategory(hwIntrinsicID) == HW_Category_Scalar)
{
return false;
}

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.

We may also want a path for HW_Category_Special or maybe better check for:

Suggested change
if (HWIntrinsicInfo::lookupCategory(hwIntrinsicID) == HW_Category_Scalar)
{
returnfalse;
}
unsigned simdSize = 0;
if (HWIntrinsicInfo::tryLookupSimdSize(hwIntrinsicID, &simdSize) && (simdSize == 0))
{
returnfalse;
}

All scalar intrinsics, regardless of category, should have a simdSize == 0. Likewise, all SIMD intrinsics should have a size of 8, 16, 32, or -1 (where -1 implies the intrinsic supports multiple SIMD sizes and is not a scalar).

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.

I think the current check is "accurate enough" today since the few "special" scalars tend to have all INS_invalid entries and are specially handled in codegen instead, but its probably better to be more explicit here.

There's also a consideration for the type encoding that certain special or helper intrinsics might specify INS_invalid and get specialized handling in lowering/codegen and so it might be important to have the type anyways. I'm unsure if there is any actual issue here or not, but its probably worth auditing at some point in the future.

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 agree, except that it doesn't set the simdSize if it was -1. So probably I should initialize simdSize to something like 100 or something and then see if became 0. Wil fix it.

@tannergoodingtannergoodingJul 28, 2022

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.

That's what the tryLookupSimdSize check was for. It should return false if simdSize == -1 and so the second check (simdSize == 0) won't get hit and we'll treat it as SIMD rather than Scalar

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.

ah. was an oversight on my part.

@kunalspathak
kunalspathak merged commit ef7c980 into dotnet:mainJul 28, 2022
@kunalspathak
kunalspathak deleted the dumpsimdType branch July 28, 2022 20:21
@ghostghost locked as resolved and limited conversation to collaborators Aug 28, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-CodeGen-coreclrCLR 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 'preciseVarTypeMap[type] != TYP_UNDEF' when dumping all functions

2 participants

@kunalspathak@tannergooding
, '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

Do not embed result type in VN for scalar hw intrinsics - #72895

Merged
kunalspathak merged 3 commits into
dotnet:mainfrom
kunalspathak:dumpsimdType
Jul 28, 2022
Merged

Do not embed result type in VN for scalar hw intrinsics#72895
kunalspathak merged 3 commits into
dotnet:mainfrom
kunalspathak:dumpsimdType

Conversation

@kunalspathak

Copy link
Copy Markdown
Contributor

For scalar hw intrinsics, we were creating VNForSimdType and it was manifesting in vnDumpSimdType().

Fixes: #69551

@ghostghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jul 26, 2022
@kunalspathak

Copy link
Copy Markdown
ContributorAuthor

@SingleAccretion , @dotnet/jit-contrib , @tannergooding

@ghost

Copy link
Copy Markdown

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

Issue Details

For scalar hw intrinsics, we were creating VNForSimdType and it was manifesting in vnDumpSimdType().

Fixes: #69551

Author:kunalspathak
Assignees:-
Labels:

area-CodeGen-coreclr

Milestone:-

@kunalspathakkunalspathak changed the title Do not embed result type for scalar hw intrinsicsDo not embed result type in VN for scalar hw intrinsicsJul 26, 2022
Comment threadsrc/coreclr/jit/hwintrinsic.cpp Outdated
Comment on lines +215 to +218
if (HWIntrinsicInfo::lookupCategory(hwIntrinsicID) == HW_Category_Scalar)
{
return false;
}

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.

We may also want a path for HW_Category_Special or maybe better check for:

Suggested change
if (HWIntrinsicInfo::lookupCategory(hwIntrinsicID) == HW_Category_Scalar)
{
returnfalse;
}
unsigned simdSize = 0;
if (HWIntrinsicInfo::tryLookupSimdSize(hwIntrinsicID, &simdSize) && (simdSize == 0))
{
returnfalse;
}

All scalar intrinsics, regardless of category, should have a simdSize == 0. Likewise, all SIMD intrinsics should have a size of 8, 16, 32, or -1 (where -1 implies the intrinsic supports multiple SIMD sizes and is not a scalar).

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.

I think the current check is "accurate enough" today since the few "special" scalars tend to have all INS_invalid entries and are specially handled in codegen instead, but its probably better to be more explicit here.

There's also a consideration for the type encoding that certain special or helper intrinsics might specify INS_invalid and get specialized handling in lowering/codegen and so it might be important to have the type anyways. I'm unsure if there is any actual issue here or not, but its probably worth auditing at some point in the future.

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 agree, except that it doesn't set the simdSize if it was -1. So probably I should initialize simdSize to something like 100 or something and then see if became 0. Wil fix it.

@tannergoodingtannergoodingJul 28, 2022

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.

That's what the tryLookupSimdSize check was for. It should return false if simdSize == -1 and so the second check (simdSize == 0) won't get hit and we'll treat it as SIMD rather than Scalar

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.

ah. was an oversight on my part.

@kunalspathak
kunalspathak merged commit ef7c980 into dotnet:mainJul 28, 2022
@kunalspathak
kunalspathak deleted the dumpsimdType branch July 28, 2022 20:21
@ghostghost locked as resolved and limited conversation to collaborators Aug 28, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-CodeGen-coreclrCLR 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 'preciseVarTypeMap[type] != TYP_UNDEF' when dumping all functions

2 participants

@kunalspathak@tannergooding
, '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

Do not embed result type in VN for scalar hw intrinsics - #72895

Merged
kunalspathak merged 3 commits into
dotnet:mainfrom
kunalspathak:dumpsimdType
Jul 28, 2022
Merged

Do not embed result type in VN for scalar hw intrinsics#72895
kunalspathak merged 3 commits into
dotnet:mainfrom
kunalspathak:dumpsimdType

Conversation

@kunalspathak

Copy link
Copy Markdown
Contributor

For scalar hw intrinsics, we were creating VNForSimdType and it was manifesting in vnDumpSimdType().

Fixes: #69551

@ghostghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jul 26, 2022
@kunalspathak

Copy link
Copy Markdown
ContributorAuthor

@SingleAccretion , @dotnet/jit-contrib , @tannergooding

@ghost

Copy link
Copy Markdown

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

Issue Details

For scalar hw intrinsics, we were creating VNForSimdType and it was manifesting in vnDumpSimdType().

Fixes: #69551

Author:kunalspathak
Assignees:-
Labels:

area-CodeGen-coreclr

Milestone:-

@kunalspathakkunalspathak changed the title Do not embed result type for scalar hw intrinsicsDo not embed result type in VN for scalar hw intrinsicsJul 26, 2022
Comment threadsrc/coreclr/jit/hwintrinsic.cpp Outdated
Comment on lines +215 to +218
if (HWIntrinsicInfo::lookupCategory(hwIntrinsicID) == HW_Category_Scalar)
{
return false;
}

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.

We may also want a path for HW_Category_Special or maybe better check for:

Suggested change
if (HWIntrinsicInfo::lookupCategory(hwIntrinsicID) == HW_Category_Scalar)
{
returnfalse;
}
unsigned simdSize = 0;
if (HWIntrinsicInfo::tryLookupSimdSize(hwIntrinsicID, &simdSize) && (simdSize == 0))
{
returnfalse;
}

All scalar intrinsics, regardless of category, should have a simdSize == 0. Likewise, all SIMD intrinsics should have a size of 8, 16, 32, or -1 (where -1 implies the intrinsic supports multiple SIMD sizes and is not a scalar).

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.

I think the current check is "accurate enough" today since the few "special" scalars tend to have all INS_invalid entries and are specially handled in codegen instead, but its probably better to be more explicit here.

There's also a consideration for the type encoding that certain special or helper intrinsics might specify INS_invalid and get specialized handling in lowering/codegen and so it might be important to have the type anyways. I'm unsure if there is any actual issue here or not, but its probably worth auditing at some point in the future.

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 agree, except that it doesn't set the simdSize if it was -1. So probably I should initialize simdSize to something like 100 or something and then see if became 0. Wil fix it.

@tannergoodingtannergoodingJul 28, 2022

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.

That's what the tryLookupSimdSize check was for. It should return false if simdSize == -1 and so the second check (simdSize == 0) won't get hit and we'll treat it as SIMD rather than Scalar

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.

ah. was an oversight on my part.

@kunalspathak
kunalspathak merged commit ef7c980 into dotnet:mainJul 28, 2022
@kunalspathak
kunalspathak deleted the dumpsimdType branch July 28, 2022 20:21
@ghostghost locked as resolved and limited conversation to collaborators Aug 28, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-CodeGen-coreclrCLR 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 'preciseVarTypeMap[type] != TYP_UNDEF' when dumping all functions

2 participants

@kunalspathak@tannergooding
, '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

Do not embed result type in VN for scalar hw intrinsics - #72895

Merged
kunalspathak merged 3 commits into
dotnet:mainfrom
kunalspathak:dumpsimdType
Jul 28, 2022
Merged

Do not embed result type in VN for scalar hw intrinsics#72895
kunalspathak merged 3 commits into
dotnet:mainfrom
kunalspathak:dumpsimdType

Conversation

@kunalspathak

Copy link
Copy Markdown
Contributor

For scalar hw intrinsics, we were creating VNForSimdType and it was manifesting in vnDumpSimdType().

Fixes: #69551

@ghostghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jul 26, 2022
@kunalspathak

Copy link
Copy Markdown
ContributorAuthor

@SingleAccretion , @dotnet/jit-contrib , @tannergooding

@ghost

Copy link
Copy Markdown

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

Issue Details

For scalar hw intrinsics, we were creating VNForSimdType and it was manifesting in vnDumpSimdType().

Fixes: #69551

Author:kunalspathak
Assignees:-
Labels:

area-CodeGen-coreclr

Milestone:-

@kunalspathakkunalspathak changed the title Do not embed result type for scalar hw intrinsicsDo not embed result type in VN for scalar hw intrinsicsJul 26, 2022
Comment threadsrc/coreclr/jit/hwintrinsic.cpp Outdated
Comment on lines +215 to +218
if (HWIntrinsicInfo::lookupCategory(hwIntrinsicID) == HW_Category_Scalar)
{
return false;
}

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.

We may also want a path for HW_Category_Special or maybe better check for:

Suggested change
if (HWIntrinsicInfo::lookupCategory(hwIntrinsicID) == HW_Category_Scalar)
{
returnfalse;
}
unsigned simdSize = 0;
if (HWIntrinsicInfo::tryLookupSimdSize(hwIntrinsicID, &simdSize) && (simdSize == 0))
{
returnfalse;
}

All scalar intrinsics, regardless of category, should have a simdSize == 0. Likewise, all SIMD intrinsics should have a size of 8, 16, 32, or -1 (where -1 implies the intrinsic supports multiple SIMD sizes and is not a scalar).

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.

I think the current check is "accurate enough" today since the few "special" scalars tend to have all INS_invalid entries and are specially handled in codegen instead, but its probably better to be more explicit here.

There's also a consideration for the type encoding that certain special or helper intrinsics might specify INS_invalid and get specialized handling in lowering/codegen and so it might be important to have the type anyways. I'm unsure if there is any actual issue here or not, but its probably worth auditing at some point in the future.

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 agree, except that it doesn't set the simdSize if it was -1. So probably I should initialize simdSize to something like 100 or something and then see if became 0. Wil fix it.

@tannergoodingtannergoodingJul 28, 2022

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.

That's what the tryLookupSimdSize check was for. It should return false if simdSize == -1 and so the second check (simdSize == 0) won't get hit and we'll treat it as SIMD rather than Scalar

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.

ah. was an oversight on my part.

@kunalspathak
kunalspathak merged commit ef7c980 into dotnet:mainJul 28, 2022
@kunalspathak
kunalspathak deleted the dumpsimdType branch July 28, 2022 20:21
@ghostghost locked as resolved and limited conversation to collaborators Aug 28, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-CodeGen-coreclrCLR 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 'preciseVarTypeMap[type] != TYP_UNDEF' when dumping all functions

2 participants

@kunalspathak@tannergooding
, '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

Do not embed result type in VN for scalar hw intrinsics - #72895

Merged
kunalspathak merged 3 commits into
dotnet:mainfrom
kunalspathak:dumpsimdType
Jul 28, 2022
Merged

Do not embed result type in VN for scalar hw intrinsics#72895
kunalspathak merged 3 commits into
dotnet:mainfrom
kunalspathak:dumpsimdType

Conversation

@kunalspathak

Copy link
Copy Markdown
Contributor

For scalar hw intrinsics, we were creating VNForSimdType and it was manifesting in vnDumpSimdType().

Fixes: #69551

@ghostghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jul 26, 2022
@kunalspathak

Copy link
Copy Markdown
ContributorAuthor

@SingleAccretion , @dotnet/jit-contrib , @tannergooding

@ghost

Copy link
Copy Markdown

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

Issue Details

For scalar hw intrinsics, we were creating VNForSimdType and it was manifesting in vnDumpSimdType().

Fixes: #69551

Author:kunalspathak
Assignees:-
Labels:

area-CodeGen-coreclr

Milestone:-

@kunalspathakkunalspathak changed the title Do not embed result type for scalar hw intrinsicsDo not embed result type in VN for scalar hw intrinsicsJul 26, 2022
Comment threadsrc/coreclr/jit/hwintrinsic.cpp Outdated
Comment on lines +215 to +218
if (HWIntrinsicInfo::lookupCategory(hwIntrinsicID) == HW_Category_Scalar)
{
return false;
}

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.

We may also want a path for HW_Category_Special or maybe better check for:

Suggested change
if (HWIntrinsicInfo::lookupCategory(hwIntrinsicID) == HW_Category_Scalar)
{
returnfalse;
}
unsigned simdSize = 0;
if (HWIntrinsicInfo::tryLookupSimdSize(hwIntrinsicID, &simdSize) && (simdSize == 0))
{
returnfalse;
}

All scalar intrinsics, regardless of category, should have a simdSize == 0. Likewise, all SIMD intrinsics should have a size of 8, 16, 32, or -1 (where -1 implies the intrinsic supports multiple SIMD sizes and is not a scalar).

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.

I think the current check is "accurate enough" today since the few "special" scalars tend to have all INS_invalid entries and are specially handled in codegen instead, but its probably better to be more explicit here.

There's also a consideration for the type encoding that certain special or helper intrinsics might specify INS_invalid and get specialized handling in lowering/codegen and so it might be important to have the type anyways. I'm unsure if there is any actual issue here or not, but its probably worth auditing at some point in the future.

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 agree, except that it doesn't set the simdSize if it was -1. So probably I should initialize simdSize to something like 100 or something and then see if became 0. Wil fix it.

@tannergoodingtannergoodingJul 28, 2022

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.

That's what the tryLookupSimdSize check was for. It should return false if simdSize == -1 and so the second check (simdSize == 0) won't get hit and we'll treat it as SIMD rather than Scalar

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.

ah. was an oversight on my part.

@kunalspathak
kunalspathak merged commit ef7c980 into dotnet:mainJul 28, 2022
@kunalspathak
kunalspathak deleted the dumpsimdType branch July 28, 2022 20:21
@ghostghost locked as resolved and limited conversation to collaborators Aug 28, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-CodeGen-coreclrCLR 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 'preciseVarTypeMap[type] != TYP_UNDEF' when dumping all functions

2 participants

@kunalspathak@tannergooding
, '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

Do not embed result type in VN for scalar hw intrinsics - #72895

Merged
kunalspathak merged 3 commits into
dotnet:mainfrom
kunalspathak:dumpsimdType
Jul 28, 2022
Merged

Do not embed result type in VN for scalar hw intrinsics#72895
kunalspathak merged 3 commits into
dotnet:mainfrom
kunalspathak:dumpsimdType

Conversation

@kunalspathak

Copy link
Copy Markdown
Contributor

For scalar hw intrinsics, we were creating VNForSimdType and it was manifesting in vnDumpSimdType().

Fixes: #69551

@ghostghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jul 26, 2022
@kunalspathak

Copy link
Copy Markdown
ContributorAuthor

@SingleAccretion , @dotnet/jit-contrib , @tannergooding

@ghost

Copy link
Copy Markdown

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

Issue Details

For scalar hw intrinsics, we were creating VNForSimdType and it was manifesting in vnDumpSimdType().

Fixes: #69551

Author:kunalspathak
Assignees:-
Labels:

area-CodeGen-coreclr

Milestone:-

@kunalspathakkunalspathak changed the title Do not embed result type for scalar hw intrinsicsDo not embed result type in VN for scalar hw intrinsicsJul 26, 2022
Comment threadsrc/coreclr/jit/hwintrinsic.cpp Outdated
Comment on lines +215 to +218
if (HWIntrinsicInfo::lookupCategory(hwIntrinsicID) == HW_Category_Scalar)
{
return false;
}

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.

We may also want a path for HW_Category_Special or maybe better check for:

Suggested change
if (HWIntrinsicInfo::lookupCategory(hwIntrinsicID) == HW_Category_Scalar)
{
returnfalse;
}
unsigned simdSize = 0;
if (HWIntrinsicInfo::tryLookupSimdSize(hwIntrinsicID, &simdSize) && (simdSize == 0))
{
returnfalse;
}

All scalar intrinsics, regardless of category, should have a simdSize == 0. Likewise, all SIMD intrinsics should have a size of 8, 16, 32, or -1 (where -1 implies the intrinsic supports multiple SIMD sizes and is not a scalar).

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.

I think the current check is "accurate enough" today since the few "special" scalars tend to have all INS_invalid entries and are specially handled in codegen instead, but its probably better to be more explicit here.

There's also a consideration for the type encoding that certain special or helper intrinsics might specify INS_invalid and get specialized handling in lowering/codegen and so it might be important to have the type anyways. I'm unsure if there is any actual issue here or not, but its probably worth auditing at some point in the future.

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 agree, except that it doesn't set the simdSize if it was -1. So probably I should initialize simdSize to something like 100 or something and then see if became 0. Wil fix it.

@tannergoodingtannergoodingJul 28, 2022

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.

That's what the tryLookupSimdSize check was for. It should return false if simdSize == -1 and so the second check (simdSize == 0) won't get hit and we'll treat it as SIMD rather than Scalar

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.

ah. was an oversight on my part.

@kunalspathak
kunalspathak merged commit ef7c980 into dotnet:mainJul 28, 2022
@kunalspathak
kunalspathak deleted the dumpsimdType branch July 28, 2022 20:21
@ghostghost locked as resolved and limited conversation to collaborators Aug 28, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-CodeGen-coreclrCLR 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 'preciseVarTypeMap[type] != TYP_UNDEF' when dumping all functions

2 participants

@kunalspathak@tannergooding