Enable assert in perfScoreUnhandledInstruction - #810

Merged
briansull merged 6 commits into
dotnet:masterfrom
briansull:perfscore-assert
Mar 3, 2020
Merged

Enable assert in perfScoreUnhandledInstruction#810
briansull merged 6 commits into
dotnet:masterfrom
briansull:perfscore-assert

Conversation

@briansull

@briansullbriansull commented Dec 12, 2019

Copy link
Copy Markdown
Contributor

perfScoreUnhandledInstruction will now assert in a DEBUG or CHECKED build when it encounters an unhanded instruction

@briansullbriansull mentioned this pull request Dec 12, 2019
Merged
@briansull

briansull commented Dec 12, 2019

Copy link
Copy Markdown
ContributorAuthor

Related to PR #751

@briansull

briansull commented Dec 12, 2019

Copy link
Copy Markdown
ContributorAuthor

@tannergooding@BruceForstall @dotnet/jit-contrib PTAL

BruceForstall
BruceForstall approved these changes Dec 12, 2019
View reviewed changes

@BruceForstallBruceForstall left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you make sure to run outerloop and outerloop stress jobs? I wouldn't want this to cause new asserts to show up, especially as we aren't actively monitoring all the various pipelines yet.

@briansull

briansull commented Dec 12, 2019

Copy link
Copy Markdown
ContributorAuthor

I am leaving this PR open as I will be leaving for a vacation shortly.

@sandreenkosandreenko added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Dec 18, 2019
@briansull

briansull commented Jan 17, 2020

Copy link
Copy Markdown
ContributorAuthor

@BruceForstall I have refreshed this

@briansull

briansull commented Jan 17, 2020

Copy link
Copy Markdown
ContributorAuthor

/AZP list

@azure-pipelines

azure-pipelinesBot commented Jan 17, 2020

Copy link
Copy Markdown
CI/CD Pipelines for this repository:

@briansull

briansull commented Jan 17, 2020

Copy link
Copy Markdown
ContributorAuthor

/AZP runtime-coreclr outerloop

@azure-pipelines

azure-pipelinesBot commented Jan 17, 2020

Copy link
Copy Markdown
Command 'runtime-coreclr' is not supported by Azure Pipelines.

Supported commands
  • help:
    • Get descriptions, examples and documentation about supported commands
    • Example: help "command_name"
  • list:
    • List all pipelines for this repository using a comment.
    • Example: "list"
  • run:
    • Run all pipelines or specific pipelines for this repository using a comment. Use this command by itself to trigger all related pipelines, or specify specific pipelines to run.
    • Example: "run" or "run pipeline_name, pipeline_name, pipeline_name"
  • where:
    • Report back the Azure DevOps orgs that are related to this repository and org
    • Example: "where"

See additional documentation.

@briansull

briansull commented Jan 17, 2020

Copy link
Copy Markdown
ContributorAuthor

/AZP run runtime-coreclr outerloop

@azure-pipelines

azure-pipelinesBot commented Jan 17, 2020

Copy link
Copy Markdown
Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

@briansull

briansull commented Jan 17, 2020

Copy link
Copy Markdown
ContributorAuthor

There are some unhanded instructions for ARM64:

https://helix.dot.net/api/2019-06-17/jobs/25335df6-3701-444a-878f-9e5140003fcd/workitems/JIT.HardwareIntrinsics/console

Assert failure(PID 220 [0x000000dc], Thread: 220 [0x00dc]): Assertion failed '!"PerfScore: unhandled instruction"' in 'JIT.HardwareIntrinsics.Arm.SimpleBinaryOpTest__AbsoluteCompareGreaterThan_Vector128_Double:RunBasicScenario_UnsafeRead():this' (IL size 114)

@tannergooding

tannergooding commented Jan 17, 2020

Copy link
Copy Markdown
Member

CC. @EgorBo, @TamarChristinaArm, @CarolEidt

@echesakov

echesakov commented Jan 17, 2020•
edited
Loading

Copy link
Copy Markdown
Contributor

@tannergooding I believe you wanted to ping me instead of Egor Bogatov. I added these instructions and I can update their perf score

@tannergooding

tannergooding commented Jan 17, 2020

Copy link
Copy Markdown
Member

Yes, sorry. Auto-complete chose the wrong person 😄

CarolEidt
CarolEidt reviewed Jan 17, 2020
View reviewed changes

@CarolEidtCarolEidt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

My preference would be not to enable this assert. I'm not in favor of asserts that appear only when dumping or disassembling.

Comment threadsrc/coreclr/src/jit/emit.cpp Outdated

@CarolEidtCarolEidtJan 17, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think that this should be deleted, and the above comment should be clarified to say that it asserts in a DEBUG build, and returns a default latency of 1 cycle otherwise.

@briansullbriansullJan 17, 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.

OK

@briansull

briansull commented Jan 17, 2020

Copy link
Copy Markdown
ContributorAuthor

People working on Arm64 instructions could just add this change-set to their enlistment to catch the missing instructions. I put this PR up because Tanner wanted to have this enabled.

@briansull

briansull commented Jan 17, 2020

Copy link
Copy Markdown
ContributorAuthor

The outer loop testing printed these two missing instructions on Arm64:

PerfScore: unhandled instruction: facgt, format IF_DV_3B
PerfScore: unhandled instruction: crc32b, format IF_DR_3A

@echesakovechesakov mentioned this pull request Jan 31, 2020
Merged
@stephentoub

stephentoub commented Feb 17, 2020

Copy link
Copy Markdown
Member

@briansull, what's the status of this PR? Should it be closed? Merged? Thanks.

@briansull

briansull commented Feb 18, 2020

Copy link
Copy Markdown
ContributorAuthor

@tannergooding do you still want this assert enabled by default?

@tannergooding

tannergooding commented Feb 18, 2020

Copy link
Copy Markdown
Member

It would be good to get it enabled as otherwise it just represents an increasing debt we are building for ARM64 (as we continue to add more instructions for the HWIntrinsic work, etc).

At this point, it should just require adding the instructions which aren't already covered, correct? Perhaps that is something that @echesakovMSFT, @CarolEidt, or myself can pick up and finish.

@CarolEidt

CarolEidt commented Feb 18, 2020

Copy link
Copy Markdown
Contributor

I'm still of the opinion that we don't want asserts that fire only when dumping or disassembling. I would be OK with a COMPlus option that enables the assert.

@tannergooding

tannergooding commented Feb 18, 2020

Copy link
Copy Markdown
Member

I'm still of the opinion that we don't want asserts that fire only when dumping or disassembling. I would be OK with a COMPlus option that enables the assert.

IIRC, this fell out of #751 (comment) and only wasn't enabled due to parallel work from the HWIntrinsics.
This PR would just be enabling the same validation we already have for x64.

@CarolEidt

CarolEidt commented Feb 18, 2020

Copy link
Copy Markdown
Contributor

My concern is with enabling an assert that only fires when I generate a dump or disassembly. This is not something we regularly do across all of our tests, and so it doesn't really add coverage. It is more likely to trip up someone who is working on something unrelated. These asserts should either be optionally enabled in dumps and disassembly (and included in some kind of stress testing) or the perfscore should always be computed for each instruction in checked builds, even when not disassembling.

@briansull

briansull commented Feb 18, 2020•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

The PerfScore is always computed for Checked and Debug builds, not only when you generate a dump or disassembly.

size_t emitter::emitIssue1Instr(insGroup* ig, instrDesc* id, BYTE** dp)
{
_..._
#if defined(DEBUG) || defined(LATE_DISASM)
float insExeCost = insEvaluateExecutionCost(id);

@CarolEidt

CarolEidt commented Feb 19, 2020

Copy link
Copy Markdown
Contributor

The PerfScore is always computed for Checked and Debug builds.

I see - I'd missed that - in that case, I'm OK with this. Sorry for the confusion.

@briansull
perfScoreUnhandledInstruction will now assert in a DEBUG or CHECKED b…
54c1e90
…uild when it encounters an unhanded instruction
@briansull

briansull commented Feb 28, 2020

Copy link
Copy Markdown
ContributorAuthor

Rebased to a recent master:

@echesakovMSFT PTAL

set COMPlus_AltJitName=protononjit.dll
set COMPLUS_AltJitNgen=*
set COMPlus_TieredCompilation=0
crossgen.exe System.Private.CoreLib.dll
PerfScore: unhandled instruction: uminv, format IF_DV_2T
Assert failure(PID 53748 [0x0000d1f4], Thread: 55356 [0xd83c]): Assertion failed '!"PerfScore: unhandled instruction"' in 'System.Numerics.Plane:Equals(System.Numerics.Plane):bool:this' during 'Generate code' (IL size 132)

@echesakov

echesakov commented Feb 28, 2020

Copy link
Copy Markdown
Contributor

@briansull I will add perf info later today - or if you want you can do this

@briansull

briansull commented Feb 28, 2020

Copy link
Copy Markdown
ContributorAuthor

@echesakovMSFT
I will look into adding them

@briansull

briansull commented Feb 28, 2020

Copy link
Copy Markdown
ContributorAuthor

Added PerfScore values for IF_DV_2T: // addv, saddlv, smaxv, sminv, uaddlv, umaxv, uminv

@echesakov

echesakov commented Feb 28, 2020

Copy link
Copy Markdown
Contributor

Added PerfScore values for IF_DV_2T: // addv, saddlv, smaxv, sminv, uaddlv, umaxv, uminv

Thank you, @briansull !

@briansull

briansull commented Feb 28, 2020

Copy link
Copy Markdown
ContributorAuthor

Add PerfScore support for fcmeq, fcmge, fcmgt, fcmle, fcmlt, fcvtl2, fcvtn, fcvtn2, fabd

@jaredparjaredpar mentioned this pull request Mar 3, 2020
Closed
@briansull
briansull merged commit c1e17e1 into dotnet:masterMar 3, 2020
@tannergooding

tannergooding commented Mar 3, 2020

Copy link
Copy Markdown
Member

Thanks @briansull!

@ghostghost locked as resolved and limited conversation to collaborators Dec 11, 2020
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@tannergoodingtannergoodingtannergooding approved these changes
+2 more reviewers
@CarolEidtCarolEidtCarolEidt left review comments
@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.

7 participants

@briansull@tannergooding@echesakov@stephentoub@CarolEidt@BruceForstall@sandreenko
, '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
Enable assert in perfScoreUnhandledInstruction - #810
Merged
briansull merged 6 commits into
dotnet:masterfrom 
briansull:perfscore-assert
Mar 3, 2020
Merged
Enable assert in perfScoreUnhandledInstruction#810
briansull merged 6 commits into
dotnet:masterfrom 
briansull:perfscore-assert
Conversation
@briansull
@briansullbriansull
commented
Dec 12, 2019

Copy link


Copy Markdown

Contributor
perfScoreUnhandledInstruction will now assert in a DEBUG or CHECKED build when it encounters an unhanded instruction
@briansullbriansull
mentioned this pull request
Dec 12, 2019
 Merged

@briansull
briansull
commented
Dec 12, 2019

Copy link


Copy Markdown

ContributorAuthor
Related to PR #751
@briansull
briansull
commented
Dec 12, 2019

Copy link


Copy Markdown

ContributorAuthor
@tannergooding@BruceForstall @dotnet/jit-contrib PTAL
BruceForstall
BruceForstall
approved these changes
Dec 12, 2019
View reviewed changes
@BruceForstallBruceForstall
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Can you make sure to run outerloop and outerloop stress jobs? I wouldn't want this to cause new asserts to show up, especially as we aren't actively monitoring all the various pipelines yet.
@briansull
briansull
commented
Dec 12, 2019

Copy link


Copy Markdown

ContributorAuthor
I am leaving this PR open as I will be leaving for a vacation shortly.
@sandreenkosandreenko
added
the 
area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
label
Dec 18, 2019
@briansull
briansull
commented
Jan 17, 2020

Copy link


Copy Markdown

ContributorAuthor
@BruceForstall I have refreshed this
@briansull
briansull
commented
Jan 17, 2020

Copy link


Copy Markdown

ContributorAuthor
/AZP list
@azure-pipelines
azure-pipelinesBot
commented
Jan 17, 2020

Copy link


Copy Markdown


CI/CD Pipelines for this repository:
@briansull
briansull
commented
Jan 17, 2020

Copy link


Copy Markdown

ContributorAuthor
/AZP runtime-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Jan 17, 2020

Copy link


Copy Markdown


Command 'runtime-coreclr' is not supported by Azure Pipelines.

Supported commands
  • help:
    • Get descriptions, examples and documentation about supported commands
    • Example: help "command_name"
  • list:
    • List all pipelines for this repository using a comment.
    • Example: "list"
  • run:
    • Run all pipelines or specific pipelines for this repository using a comment. Use this command by itself to trigger all related pipelines, or specify specific pipelines to run.
    • Example: "run" or "run pipeline_name, pipeline_name, pipeline_name"
  • where:
    • Report back the Azure DevOps orgs that are related to this repository and org
    • Example: "where"


See additional documentation.
@briansull
briansull
commented
Jan 17, 2020

Copy link


Copy Markdown

ContributorAuthor
/AZP run runtime-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Jan 17, 2020

Copy link


Copy Markdown


Azure Pipelines could not run because the pipeline triggers exclude this branch/path.
@briansull
briansull
commented
Jan 17, 2020

Copy link


Copy Markdown

ContributorAuthor
There are some unhanded instructions for ARM64:
https://helix.dot.net/api/2019-06-17/jobs/25335df6-3701-444a-878f-9e5140003fcd/workitems/JIT.HardwareIntrinsics/console
Assert failure(PID 220 [0x000000dc], Thread: 220 [0x00dc]): Assertion failed '!"PerfScore: unhandled instruction"' in 'JIT.HardwareIntrinsics.Arm.SimpleBinaryOpTest__AbsoluteCompareGreaterThan_Vector128_Double:RunBasicScenario_UnsafeRead():this' (IL size 114)
@tannergooding
tannergooding
commented
Jan 17, 2020

Copy link


Copy Markdown

Member
CC. @EgorBo, @TamarChristinaArm, @CarolEidt
@echesakov
echesakov
commented
Jan 17, 2020•

edited

Loading

Copy link


Copy Markdown

Contributor
@tannergooding I believe you wanted to ping me instead of Egor Bogatov. I added these instructions and I can update their perf score
@tannergooding
tannergooding
commented
Jan 17, 2020

Copy link


Copy Markdown

Member
Yes, sorry. Auto-complete chose the wrong person 😄
CarolEidt
CarolEidt
reviewed
Jan 17, 2020
View reviewed changes
@CarolEidtCarolEidt
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

My preference would be not to enable this assert. I'm not in favor of asserts that appear only when dumping or disassembling.
Comment threadsrc/coreclr/src/jit/emit.cpp
Outdated

@CarolEidtCarolEidtJan 17, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I think that this should be deleted, and the above comment should be clarified to say that it asserts in a DEBUG build, and returns a default latency of 1 cycle otherwise.
@briansullbriansullJan 17, 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.

OK
@briansull
briansull
commented
Jan 17, 2020

Copy link


Copy Markdown

ContributorAuthor
People working on Arm64 instructions could just add this change-set to their enlistment to catch the missing instructions. I put this PR up because Tanner wanted to have this enabled.
@briansull
briansull
commented
Jan 17, 2020

Copy link


Copy Markdown

ContributorAuthor
The outer loop testing printed these two missing instructions on Arm64:
PerfScore: unhandled instruction: facgt, format IF_DV_3B
PerfScore: unhandled instruction: crc32b, format IF_DR_3A

@echesakovechesakov
mentioned this pull request
Jan 31, 2020
 Merged

@stephentoub
stephentoub
commented
Feb 17, 2020

Copy link


Copy Markdown

Member
@briansull, what's the status of this PR? Should it be closed? Merged? Thanks.
@briansull
briansull
commented
Feb 18, 2020

Copy link


Copy Markdown

ContributorAuthor
@tannergooding do you still want this assert enabled by default?
@tannergooding
tannergooding
commented
Feb 18, 2020

Copy link


Copy Markdown

Member
It would be good to get it enabled as otherwise it just represents an increasing debt we are building for ARM64 (as we continue to add more instructions for the HWIntrinsic work, etc).
At this point, it should just require adding the instructions which aren't already covered, correct? Perhaps that is something that @echesakovMSFT, @CarolEidt, or myself can pick up and finish.
@CarolEidt
CarolEidt
commented
Feb 18, 2020

Copy link


Copy Markdown

Contributor
I'm still of the opinion that we don't want asserts that fire only when dumping or disassembling. I would be OK with a COMPlus option that enables the assert.
@tannergooding
tannergooding
commented
Feb 18, 2020

Copy link


Copy Markdown

Member
I'm still of the opinion that we don't want asserts that fire only when dumping or disassembling. I would be OK with a COMPlus option that enables the assert.
IIRC, this fell out of #751 (comment) and only wasn't enabled due to parallel work from the HWIntrinsics.

This PR would just be enabling the same validation we already have for x64.
@CarolEidt
CarolEidt
commented
Feb 18, 2020

Copy link


Copy Markdown

Contributor
My concern is with enabling an assert that only fires when I generate a dump or disassembly. This is not something we regularly do across all of our tests, and so it doesn't really add coverage. It is more likely to trip up someone who is working on something unrelated. These asserts should either be optionally enabled in dumps and disassembly (and included in some kind of stress testing) or the perfscore should always be computed for each instruction in checked builds, even when not disassembling.
@briansull
briansull
commented
Feb 18, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
The PerfScore is always computed for Checked and Debug builds, not only when you generate a dump or disassembly.
size_t emitter::emitIssue1Instr(insGroup* ig, instrDesc* id, BYTE** dp)
{
_..._
#if defined(DEBUG) || defined(LATE_DISASM)
float insExeCost = insEvaluateExecutionCost(id);

@CarolEidt
CarolEidt
commented
Feb 19, 2020

Copy link


Copy Markdown

Contributor
The PerfScore is always computed for Checked and Debug builds.
I see - I'd missed that - in that case, I'm OK with this. Sorry for the confusion.
@briansull
perfScoreUnhandledInstruction will now assert in a DEBUG or CHECKED b…
54c1e90
…uild when it encounters an unhanded instruction
@briansull
briansull
commented
Feb 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Rebased to a recent master:
@echesakovMSFT PTAL
set COMPlus_AltJitName=protononjit.dll
set COMPLUS_AltJitNgen=*
set COMPlus_TieredCompilation=0
crossgen.exe System.Private.CoreLib.dll
PerfScore: unhandled instruction: uminv, format IF_DV_2T
Assert failure(PID 53748 [0x0000d1f4], Thread: 55356 [0xd83c]): Assertion failed '!"PerfScore: unhandled instruction"' in 'System.Numerics.Plane:Equals(System.Numerics.Plane):bool:this' during 'Generate code' (IL size 132)

@echesakov
echesakov
commented
Feb 28, 2020

Copy link


Copy Markdown

Contributor
@briansull I will add perf info later today - or if you want you can do this
@briansull
briansull
commented
Feb 28, 2020

Copy link


Copy Markdown

ContributorAuthor
@echesakovMSFT

I will look into adding them
@briansull
briansull
commented
Feb 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Added PerfScore values for IF_DV_2T: // addv, saddlv, smaxv, sminv, uaddlv, umaxv, uminv
@echesakov
echesakov
commented
Feb 28, 2020

Copy link


Copy Markdown

Contributor
Added PerfScore values for IF_DV_2T: // addv, saddlv, smaxv, sminv, uaddlv, umaxv, uminv
Thank you, @briansull !
@briansull
briansull
commented
Feb 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Add PerfScore support for fcmeq, fcmge, fcmgt, fcmle, fcmlt, fcvtl2, fcvtn, fcvtn2, fabd
@jaredparjaredpar
mentioned this pull request
Mar 3, 2020

Closed

@briansull
briansull
merged commit c1e17e1
into
dotnet:masterMar 3, 2020
@tannergooding
tannergooding
commented
Mar 3, 2020

Copy link


Copy Markdown

Member
Thanks @briansull!
@ghostghost
locked as resolved and limited conversation to collaborators
Dec 11, 2020
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@tannergoodingtannergoodingtannergooding approved these changes

+2 more reviewers

@CarolEidtCarolEidtCarolEidt left review comments
@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.

7 participants

@briansull@tannergooding@echesakov@stephentoub@CarolEidt@BruceForstall@sandreenko

, '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
Enable assert in perfScoreUnhandledInstruction - #810
Merged
briansull merged 6 commits into
dotnet:masterfrom 
briansull:perfscore-assert
Mar 3, 2020
Merged
Enable assert in perfScoreUnhandledInstruction#810
briansull merged 6 commits into
dotnet:masterfrom 
briansull:perfscore-assert
Conversation
@briansull
@briansullbriansull
commented
Dec 12, 2019

Copy link


Copy Markdown

Contributor
perfScoreUnhandledInstruction will now assert in a DEBUG or CHECKED build when it encounters an unhanded instruction
@briansullbriansull
mentioned this pull request
Dec 12, 2019
 Merged

@briansull
briansull
commented
Dec 12, 2019

Copy link


Copy Markdown

ContributorAuthor
Related to PR #751
@briansull
briansull
commented
Dec 12, 2019

Copy link


Copy Markdown

ContributorAuthor
@tannergooding@BruceForstall @dotnet/jit-contrib PTAL
BruceForstall
BruceForstall
approved these changes
Dec 12, 2019
View reviewed changes
@BruceForstallBruceForstall
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Can you make sure to run outerloop and outerloop stress jobs? I wouldn't want this to cause new asserts to show up, especially as we aren't actively monitoring all the various pipelines yet.
@briansull
briansull
commented
Dec 12, 2019

Copy link


Copy Markdown

ContributorAuthor
I am leaving this PR open as I will be leaving for a vacation shortly.
@sandreenkosandreenko
added
the 
area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
label
Dec 18, 2019
@briansull
briansull
commented
Jan 17, 2020

Copy link


Copy Markdown

ContributorAuthor
@BruceForstall I have refreshed this
@briansull
briansull
commented
Jan 17, 2020

Copy link


Copy Markdown

ContributorAuthor
/AZP list
@azure-pipelines
azure-pipelinesBot
commented
Jan 17, 2020

Copy link


Copy Markdown


CI/CD Pipelines for this repository:
@briansull
briansull
commented
Jan 17, 2020

Copy link


Copy Markdown

ContributorAuthor
/AZP runtime-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Jan 17, 2020

Copy link


Copy Markdown


Command 'runtime-coreclr' is not supported by Azure Pipelines.

Supported commands
  • help:
    • Get descriptions, examples and documentation about supported commands
    • Example: help "command_name"
  • list:
    • List all pipelines for this repository using a comment.
    • Example: "list"
  • run:
    • Run all pipelines or specific pipelines for this repository using a comment. Use this command by itself to trigger all related pipelines, or specify specific pipelines to run.
    • Example: "run" or "run pipeline_name, pipeline_name, pipeline_name"
  • where:
    • Report back the Azure DevOps orgs that are related to this repository and org
    • Example: "where"


See additional documentation.
@briansull
briansull
commented
Jan 17, 2020

Copy link


Copy Markdown

ContributorAuthor
/AZP run runtime-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Jan 17, 2020

Copy link


Copy Markdown


Azure Pipelines could not run because the pipeline triggers exclude this branch/path.
@briansull
briansull
commented
Jan 17, 2020

Copy link


Copy Markdown

ContributorAuthor
There are some unhanded instructions for ARM64:
https://helix.dot.net/api/2019-06-17/jobs/25335df6-3701-444a-878f-9e5140003fcd/workitems/JIT.HardwareIntrinsics/console
Assert failure(PID 220 [0x000000dc], Thread: 220 [0x00dc]): Assertion failed '!"PerfScore: unhandled instruction"' in 'JIT.HardwareIntrinsics.Arm.SimpleBinaryOpTest__AbsoluteCompareGreaterThan_Vector128_Double:RunBasicScenario_UnsafeRead():this' (IL size 114)
@tannergooding
tannergooding
commented
Jan 17, 2020

Copy link


Copy Markdown

Member
CC. @EgorBo, @TamarChristinaArm, @CarolEidt
@echesakov
echesakov
commented
Jan 17, 2020•

edited

Loading

Copy link


Copy Markdown

Contributor
@tannergooding I believe you wanted to ping me instead of Egor Bogatov. I added these instructions and I can update their perf score
@tannergooding
tannergooding
commented
Jan 17, 2020

Copy link


Copy Markdown

Member
Yes, sorry. Auto-complete chose the wrong person 😄
CarolEidt
CarolEidt
reviewed
Jan 17, 2020
View reviewed changes
@CarolEidtCarolEidt
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

My preference would be not to enable this assert. I'm not in favor of asserts that appear only when dumping or disassembling.
Comment threadsrc/coreclr/src/jit/emit.cpp
Outdated

@CarolEidtCarolEidtJan 17, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I think that this should be deleted, and the above comment should be clarified to say that it asserts in a DEBUG build, and returns a default latency of 1 cycle otherwise.
@briansullbriansullJan 17, 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.

OK
@briansull
briansull
commented
Jan 17, 2020

Copy link


Copy Markdown

ContributorAuthor
People working on Arm64 instructions could just add this change-set to their enlistment to catch the missing instructions. I put this PR up because Tanner wanted to have this enabled.
@briansull
briansull
commented
Jan 17, 2020

Copy link


Copy Markdown

ContributorAuthor
The outer loop testing printed these two missing instructions on Arm64:
PerfScore: unhandled instruction: facgt, format IF_DV_3B
PerfScore: unhandled instruction: crc32b, format IF_DR_3A

@echesakovechesakov
mentioned this pull request
Jan 31, 2020
 Merged

@stephentoub
stephentoub
commented
Feb 17, 2020

Copy link


Copy Markdown

Member
@briansull, what's the status of this PR? Should it be closed? Merged? Thanks.
@briansull
briansull
commented
Feb 18, 2020

Copy link


Copy Markdown

ContributorAuthor
@tannergooding do you still want this assert enabled by default?
@tannergooding
tannergooding
commented
Feb 18, 2020

Copy link


Copy Markdown

Member
It would be good to get it enabled as otherwise it just represents an increasing debt we are building for ARM64 (as we continue to add more instructions for the HWIntrinsic work, etc).
At this point, it should just require adding the instructions which aren't already covered, correct? Perhaps that is something that @echesakovMSFT, @CarolEidt, or myself can pick up and finish.
@CarolEidt
CarolEidt
commented
Feb 18, 2020

Copy link


Copy Markdown

Contributor
I'm still of the opinion that we don't want asserts that fire only when dumping or disassembling. I would be OK with a COMPlus option that enables the assert.
@tannergooding
tannergooding
commented
Feb 18, 2020

Copy link


Copy Markdown

Member
I'm still of the opinion that we don't want asserts that fire only when dumping or disassembling. I would be OK with a COMPlus option that enables the assert.
IIRC, this fell out of #751 (comment) and only wasn't enabled due to parallel work from the HWIntrinsics.

This PR would just be enabling the same validation we already have for x64.
@CarolEidt
CarolEidt
commented
Feb 18, 2020

Copy link


Copy Markdown

Contributor
My concern is with enabling an assert that only fires when I generate a dump or disassembly. This is not something we regularly do across all of our tests, and so it doesn't really add coverage. It is more likely to trip up someone who is working on something unrelated. These asserts should either be optionally enabled in dumps and disassembly (and included in some kind of stress testing) or the perfscore should always be computed for each instruction in checked builds, even when not disassembling.
@briansull
briansull
commented
Feb 18, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
The PerfScore is always computed for Checked and Debug builds, not only when you generate a dump or disassembly.
size_t emitter::emitIssue1Instr(insGroup* ig, instrDesc* id, BYTE** dp)
{
_..._
#if defined(DEBUG) || defined(LATE_DISASM)
float insExeCost = insEvaluateExecutionCost(id);

@CarolEidt
CarolEidt
commented
Feb 19, 2020

Copy link


Copy Markdown

Contributor
The PerfScore is always computed for Checked and Debug builds.
I see - I'd missed that - in that case, I'm OK with this. Sorry for the confusion.
@briansull
perfScoreUnhandledInstruction will now assert in a DEBUG or CHECKED b…
54c1e90
…uild when it encounters an unhanded instruction
@briansull
briansull
commented
Feb 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Rebased to a recent master:
@echesakovMSFT PTAL
set COMPlus_AltJitName=protononjit.dll
set COMPLUS_AltJitNgen=*
set COMPlus_TieredCompilation=0
crossgen.exe System.Private.CoreLib.dll
PerfScore: unhandled instruction: uminv, format IF_DV_2T
Assert failure(PID 53748 [0x0000d1f4], Thread: 55356 [0xd83c]): Assertion failed '!"PerfScore: unhandled instruction"' in 'System.Numerics.Plane:Equals(System.Numerics.Plane):bool:this' during 'Generate code' (IL size 132)

@echesakov
echesakov
commented
Feb 28, 2020

Copy link


Copy Markdown

Contributor
@briansull I will add perf info later today - or if you want you can do this
@briansull
briansull
commented
Feb 28, 2020

Copy link


Copy Markdown

ContributorAuthor
@echesakovMSFT

I will look into adding them
@briansull
briansull
commented
Feb 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Added PerfScore values for IF_DV_2T: // addv, saddlv, smaxv, sminv, uaddlv, umaxv, uminv
@echesakov
echesakov
commented
Feb 28, 2020

Copy link


Copy Markdown

Contributor
Added PerfScore values for IF_DV_2T: // addv, saddlv, smaxv, sminv, uaddlv, umaxv, uminv
Thank you, @briansull !
@briansull
briansull
commented
Feb 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Add PerfScore support for fcmeq, fcmge, fcmgt, fcmle, fcmlt, fcvtl2, fcvtn, fcvtn2, fabd
@jaredparjaredpar
mentioned this pull request
Mar 3, 2020

Closed

@briansull
briansull
merged commit c1e17e1
into
dotnet:masterMar 3, 2020
@tannergooding
tannergooding
commented
Mar 3, 2020

Copy link


Copy Markdown

Member
Thanks @briansull!
@ghostghost
locked as resolved and limited conversation to collaborators
Dec 11, 2020
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@tannergoodingtannergoodingtannergooding approved these changes

+2 more reviewers

@CarolEidtCarolEidtCarolEidt left review comments
@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.

7 participants

@briansull@tannergooding@echesakov@stephentoub@CarolEidt@BruceForstall@sandreenko

, '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
Enable assert in perfScoreUnhandledInstruction - #810
Merged
briansull merged 6 commits into
dotnet:masterfrom 
briansull:perfscore-assert
Mar 3, 2020
Merged
Enable assert in perfScoreUnhandledInstruction#810
briansull merged 6 commits into
dotnet:masterfrom 
briansull:perfscore-assert
Conversation
@briansull
@briansullbriansull
commented
Dec 12, 2019

Copy link


Copy Markdown

Contributor
perfScoreUnhandledInstruction will now assert in a DEBUG or CHECKED build when it encounters an unhanded instruction
@briansullbriansull
mentioned this pull request
Dec 12, 2019
 Merged

@briansull
briansull
commented
Dec 12, 2019

Copy link


Copy Markdown

ContributorAuthor
Related to PR #751
@briansull
briansull
commented
Dec 12, 2019

Copy link


Copy Markdown

ContributorAuthor
@tannergooding@BruceForstall @dotnet/jit-contrib PTAL
BruceForstall
BruceForstall
approved these changes
Dec 12, 2019
View reviewed changes
@BruceForstallBruceForstall
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Can you make sure to run outerloop and outerloop stress jobs? I wouldn't want this to cause new asserts to show up, especially as we aren't actively monitoring all the various pipelines yet.
@briansull
briansull
commented
Dec 12, 2019

Copy link


Copy Markdown

ContributorAuthor
I am leaving this PR open as I will be leaving for a vacation shortly.
@sandreenkosandreenko
added
the 
area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
label
Dec 18, 2019
@briansull
briansull
commented
Jan 17, 2020

Copy link


Copy Markdown

ContributorAuthor
@BruceForstall I have refreshed this
@briansull
briansull
commented
Jan 17, 2020

Copy link


Copy Markdown

ContributorAuthor
/AZP list
@azure-pipelines
azure-pipelinesBot
commented
Jan 17, 2020

Copy link


Copy Markdown


CI/CD Pipelines for this repository:
@briansull
briansull
commented
Jan 17, 2020

Copy link


Copy Markdown

ContributorAuthor
/AZP runtime-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Jan 17, 2020

Copy link


Copy Markdown


Command 'runtime-coreclr' is not supported by Azure Pipelines.

Supported commands
  • help:
    • Get descriptions, examples and documentation about supported commands
    • Example: help "command_name"
  • list:
    • List all pipelines for this repository using a comment.
    • Example: "list"
  • run:
    • Run all pipelines or specific pipelines for this repository using a comment. Use this command by itself to trigger all related pipelines, or specify specific pipelines to run.
    • Example: "run" or "run pipeline_name, pipeline_name, pipeline_name"
  • where:
    • Report back the Azure DevOps orgs that are related to this repository and org
    • Example: "where"


See additional documentation.
@briansull
briansull
commented
Jan 17, 2020

Copy link


Copy Markdown

ContributorAuthor
/AZP run runtime-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Jan 17, 2020

Copy link


Copy Markdown


Azure Pipelines could not run because the pipeline triggers exclude this branch/path.
@briansull
briansull
commented
Jan 17, 2020

Copy link


Copy Markdown

ContributorAuthor
There are some unhanded instructions for ARM64:
https://helix.dot.net/api/2019-06-17/jobs/25335df6-3701-444a-878f-9e5140003fcd/workitems/JIT.HardwareIntrinsics/console
Assert failure(PID 220 [0x000000dc], Thread: 220 [0x00dc]): Assertion failed '!"PerfScore: unhandled instruction"' in 'JIT.HardwareIntrinsics.Arm.SimpleBinaryOpTest__AbsoluteCompareGreaterThan_Vector128_Double:RunBasicScenario_UnsafeRead():this' (IL size 114)
@tannergooding
tannergooding
commented
Jan 17, 2020

Copy link


Copy Markdown

Member
CC. @EgorBo, @TamarChristinaArm, @CarolEidt
@echesakov
echesakov
commented
Jan 17, 2020•

edited

Loading

Copy link


Copy Markdown

Contributor
@tannergooding I believe you wanted to ping me instead of Egor Bogatov. I added these instructions and I can update their perf score
@tannergooding
tannergooding
commented
Jan 17, 2020

Copy link


Copy Markdown

Member
Yes, sorry. Auto-complete chose the wrong person 😄
CarolEidt
CarolEidt
reviewed
Jan 17, 2020
View reviewed changes
@CarolEidtCarolEidt
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

My preference would be not to enable this assert. I'm not in favor of asserts that appear only when dumping or disassembling.
Comment threadsrc/coreclr/src/jit/emit.cpp
Outdated

@CarolEidtCarolEidtJan 17, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I think that this should be deleted, and the above comment should be clarified to say that it asserts in a DEBUG build, and returns a default latency of 1 cycle otherwise.
@briansullbriansullJan 17, 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.

OK
@briansull
briansull
commented
Jan 17, 2020

Copy link


Copy Markdown

ContributorAuthor
People working on Arm64 instructions could just add this change-set to their enlistment to catch the missing instructions. I put this PR up because Tanner wanted to have this enabled.
@briansull
briansull
commented
Jan 17, 2020

Copy link


Copy Markdown

ContributorAuthor
The outer loop testing printed these two missing instructions on Arm64:
PerfScore: unhandled instruction: facgt, format IF_DV_3B
PerfScore: unhandled instruction: crc32b, format IF_DR_3A

@echesakovechesakov
mentioned this pull request
Jan 31, 2020
 Merged

@stephentoub
stephentoub
commented
Feb 17, 2020

Copy link


Copy Markdown

Member
@briansull, what's the status of this PR? Should it be closed? Merged? Thanks.
@briansull
briansull
commented
Feb 18, 2020

Copy link


Copy Markdown

ContributorAuthor
@tannergooding do you still want this assert enabled by default?
@tannergooding
tannergooding
commented
Feb 18, 2020

Copy link


Copy Markdown

Member
It would be good to get it enabled as otherwise it just represents an increasing debt we are building for ARM64 (as we continue to add more instructions for the HWIntrinsic work, etc).
At this point, it should just require adding the instructions which aren't already covered, correct? Perhaps that is something that @echesakovMSFT, @CarolEidt, or myself can pick up and finish.
@CarolEidt
CarolEidt
commented
Feb 18, 2020

Copy link


Copy Markdown

Contributor
I'm still of the opinion that we don't want asserts that fire only when dumping or disassembling. I would be OK with a COMPlus option that enables the assert.
@tannergooding
tannergooding
commented
Feb 18, 2020

Copy link


Copy Markdown

Member
I'm still of the opinion that we don't want asserts that fire only when dumping or disassembling. I would be OK with a COMPlus option that enables the assert.
IIRC, this fell out of #751 (comment) and only wasn't enabled due to parallel work from the HWIntrinsics.

This PR would just be enabling the same validation we already have for x64.
@CarolEidt
CarolEidt
commented
Feb 18, 2020

Copy link


Copy Markdown

Contributor
My concern is with enabling an assert that only fires when I generate a dump or disassembly. This is not something we regularly do across all of our tests, and so it doesn't really add coverage. It is more likely to trip up someone who is working on something unrelated. These asserts should either be optionally enabled in dumps and disassembly (and included in some kind of stress testing) or the perfscore should always be computed for each instruction in checked builds, even when not disassembling.
@briansull
briansull
commented
Feb 18, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
The PerfScore is always computed for Checked and Debug builds, not only when you generate a dump or disassembly.
size_t emitter::emitIssue1Instr(insGroup* ig, instrDesc* id, BYTE** dp)
{
_..._
#if defined(DEBUG) || defined(LATE_DISASM)
float insExeCost = insEvaluateExecutionCost(id);

@CarolEidt
CarolEidt
commented
Feb 19, 2020

Copy link


Copy Markdown

Contributor
The PerfScore is always computed for Checked and Debug builds.
I see - I'd missed that - in that case, I'm OK with this. Sorry for the confusion.
@briansull
perfScoreUnhandledInstruction will now assert in a DEBUG or CHECKED b…
54c1e90
…uild when it encounters an unhanded instruction
@briansull
briansull
commented
Feb 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Rebased to a recent master:
@echesakovMSFT PTAL
set COMPlus_AltJitName=protononjit.dll
set COMPLUS_AltJitNgen=*
set COMPlus_TieredCompilation=0
crossgen.exe System.Private.CoreLib.dll
PerfScore: unhandled instruction: uminv, format IF_DV_2T
Assert failure(PID 53748 [0x0000d1f4], Thread: 55356 [0xd83c]): Assertion failed '!"PerfScore: unhandled instruction"' in 'System.Numerics.Plane:Equals(System.Numerics.Plane):bool:this' during 'Generate code' (IL size 132)

@echesakov
echesakov
commented
Feb 28, 2020

Copy link


Copy Markdown

Contributor
@briansull I will add perf info later today - or if you want you can do this
@briansull
briansull
commented
Feb 28, 2020

Copy link


Copy Markdown

ContributorAuthor
@echesakovMSFT

I will look into adding them
@briansull
briansull
commented
Feb 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Added PerfScore values for IF_DV_2T: // addv, saddlv, smaxv, sminv, uaddlv, umaxv, uminv
@echesakov
echesakov
commented
Feb 28, 2020

Copy link


Copy Markdown

Contributor
Added PerfScore values for IF_DV_2T: // addv, saddlv, smaxv, sminv, uaddlv, umaxv, uminv
Thank you, @briansull !
@briansull
briansull
commented
Feb 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Add PerfScore support for fcmeq, fcmge, fcmgt, fcmle, fcmlt, fcvtl2, fcvtn, fcvtn2, fabd
@jaredparjaredpar
mentioned this pull request
Mar 3, 2020

Closed

@briansull
briansull
merged commit c1e17e1
into
dotnet:masterMar 3, 2020
@tannergooding
tannergooding
commented
Mar 3, 2020

Copy link


Copy Markdown

Member
Thanks @briansull!
@ghostghost
locked as resolved and limited conversation to collaborators
Dec 11, 2020
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@tannergoodingtannergoodingtannergooding approved these changes

+2 more reviewers

@CarolEidtCarolEidtCarolEidt left review comments
@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.

7 participants

@briansull@tannergooding@echesakov@stephentoub@CarolEidt@BruceForstall@sandreenko

, '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
Enable assert in perfScoreUnhandledInstruction - #810
Merged
briansull merged 6 commits into
dotnet:masterfrom 
briansull:perfscore-assert
Mar 3, 2020
Merged
Enable assert in perfScoreUnhandledInstruction#810
briansull merged 6 commits into
dotnet:masterfrom 
briansull:perfscore-assert
Conversation
@briansull
@briansullbriansull
commented
Dec 12, 2019

Copy link


Copy Markdown

Contributor
perfScoreUnhandledInstruction will now assert in a DEBUG or CHECKED build when it encounters an unhanded instruction
@briansullbriansull
mentioned this pull request
Dec 12, 2019
 Merged

@briansull
briansull
commented
Dec 12, 2019

Copy link


Copy Markdown

ContributorAuthor
Related to PR #751
@briansull
briansull
commented
Dec 12, 2019

Copy link


Copy Markdown

ContributorAuthor
@tannergooding@BruceForstall @dotnet/jit-contrib PTAL
BruceForstall
BruceForstall
approved these changes
Dec 12, 2019
View reviewed changes
@BruceForstallBruceForstall
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Can you make sure to run outerloop and outerloop stress jobs? I wouldn't want this to cause new asserts to show up, especially as we aren't actively monitoring all the various pipelines yet.
@briansull
briansull
commented
Dec 12, 2019

Copy link


Copy Markdown

ContributorAuthor
I am leaving this PR open as I will be leaving for a vacation shortly.
@sandreenkosandreenko
added
the 
area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
label
Dec 18, 2019
@briansull
briansull
commented
Jan 17, 2020

Copy link


Copy Markdown

ContributorAuthor
@BruceForstall I have refreshed this
@briansull
briansull
commented
Jan 17, 2020

Copy link


Copy Markdown

ContributorAuthor
/AZP list
@azure-pipelines
azure-pipelinesBot
commented
Jan 17, 2020

Copy link


Copy Markdown


CI/CD Pipelines for this repository:
@briansull
briansull
commented
Jan 17, 2020

Copy link


Copy Markdown

ContributorAuthor
/AZP runtime-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Jan 17, 2020

Copy link


Copy Markdown


Command 'runtime-coreclr' is not supported by Azure Pipelines.

Supported commands
  • help:
    • Get descriptions, examples and documentation about supported commands
    • Example: help "command_name"
  • list:
    • List all pipelines for this repository using a comment.
    • Example: "list"
  • run:
    • Run all pipelines or specific pipelines for this repository using a comment. Use this command by itself to trigger all related pipelines, or specify specific pipelines to run.
    • Example: "run" or "run pipeline_name, pipeline_name, pipeline_name"
  • where:
    • Report back the Azure DevOps orgs that are related to this repository and org
    • Example: "where"


See additional documentation.
@briansull
briansull
commented
Jan 17, 2020

Copy link


Copy Markdown

ContributorAuthor
/AZP run runtime-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Jan 17, 2020

Copy link


Copy Markdown


Azure Pipelines could not run because the pipeline triggers exclude this branch/path.
@briansull
briansull
commented
Jan 17, 2020

Copy link


Copy Markdown

ContributorAuthor
There are some unhanded instructions for ARM64:
https://helix.dot.net/api/2019-06-17/jobs/25335df6-3701-444a-878f-9e5140003fcd/workitems/JIT.HardwareIntrinsics/console
Assert failure(PID 220 [0x000000dc], Thread: 220 [0x00dc]): Assertion failed '!"PerfScore: unhandled instruction"' in 'JIT.HardwareIntrinsics.Arm.SimpleBinaryOpTest__AbsoluteCompareGreaterThan_Vector128_Double:RunBasicScenario_UnsafeRead():this' (IL size 114)
@tannergooding
tannergooding
commented
Jan 17, 2020

Copy link


Copy Markdown

Member
CC. @EgorBo, @TamarChristinaArm, @CarolEidt
@echesakov
echesakov
commented
Jan 17, 2020•

edited

Loading

Copy link


Copy Markdown

Contributor
@tannergooding I believe you wanted to ping me instead of Egor Bogatov. I added these instructions and I can update their perf score
@tannergooding
tannergooding
commented
Jan 17, 2020

Copy link


Copy Markdown

Member
Yes, sorry. Auto-complete chose the wrong person 😄
CarolEidt
CarolEidt
reviewed
Jan 17, 2020
View reviewed changes
@CarolEidtCarolEidt
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

My preference would be not to enable this assert. I'm not in favor of asserts that appear only when dumping or disassembling.
Comment threadsrc/coreclr/src/jit/emit.cpp
Outdated

@CarolEidtCarolEidtJan 17, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I think that this should be deleted, and the above comment should be clarified to say that it asserts in a DEBUG build, and returns a default latency of 1 cycle otherwise.
@briansullbriansullJan 17, 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.

OK
@briansull
briansull
commented
Jan 17, 2020

Copy link


Copy Markdown

ContributorAuthor
People working on Arm64 instructions could just add this change-set to their enlistment to catch the missing instructions. I put this PR up because Tanner wanted to have this enabled.
@briansull
briansull
commented
Jan 17, 2020

Copy link


Copy Markdown

ContributorAuthor
The outer loop testing printed these two missing instructions on Arm64:
PerfScore: unhandled instruction: facgt, format IF_DV_3B
PerfScore: unhandled instruction: crc32b, format IF_DR_3A

@echesakovechesakov
mentioned this pull request
Jan 31, 2020
 Merged

@stephentoub
stephentoub
commented
Feb 17, 2020

Copy link


Copy Markdown

Member
@briansull, what's the status of this PR? Should it be closed? Merged? Thanks.
@briansull
briansull
commented
Feb 18, 2020

Copy link


Copy Markdown

ContributorAuthor
@tannergooding do you still want this assert enabled by default?
@tannergooding
tannergooding
commented
Feb 18, 2020

Copy link


Copy Markdown

Member
It would be good to get it enabled as otherwise it just represents an increasing debt we are building for ARM64 (as we continue to add more instructions for the HWIntrinsic work, etc).
At this point, it should just require adding the instructions which aren't already covered, correct? Perhaps that is something that @echesakovMSFT, @CarolEidt, or myself can pick up and finish.
@CarolEidt
CarolEidt
commented
Feb 18, 2020

Copy link


Copy Markdown

Contributor
I'm still of the opinion that we don't want asserts that fire only when dumping or disassembling. I would be OK with a COMPlus option that enables the assert.
@tannergooding
tannergooding
commented
Feb 18, 2020

Copy link


Copy Markdown

Member
I'm still of the opinion that we don't want asserts that fire only when dumping or disassembling. I would be OK with a COMPlus option that enables the assert.
IIRC, this fell out of #751 (comment) and only wasn't enabled due to parallel work from the HWIntrinsics.

This PR would just be enabling the same validation we already have for x64.
@CarolEidt
CarolEidt
commented
Feb 18, 2020

Copy link


Copy Markdown

Contributor
My concern is with enabling an assert that only fires when I generate a dump or disassembly. This is not something we regularly do across all of our tests, and so it doesn't really add coverage. It is more likely to trip up someone who is working on something unrelated. These asserts should either be optionally enabled in dumps and disassembly (and included in some kind of stress testing) or the perfscore should always be computed for each instruction in checked builds, even when not disassembling.
@briansull
briansull
commented
Feb 18, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
The PerfScore is always computed for Checked and Debug builds, not only when you generate a dump or disassembly.
size_t emitter::emitIssue1Instr(insGroup* ig, instrDesc* id, BYTE** dp)
{
_..._
#if defined(DEBUG) || defined(LATE_DISASM)
float insExeCost = insEvaluateExecutionCost(id);

@CarolEidt
CarolEidt
commented
Feb 19, 2020

Copy link


Copy Markdown

Contributor
The PerfScore is always computed for Checked and Debug builds.
I see - I'd missed that - in that case, I'm OK with this. Sorry for the confusion.
@briansull
perfScoreUnhandledInstruction will now assert in a DEBUG or CHECKED b…
54c1e90
…uild when it encounters an unhanded instruction
@briansull
briansull
commented
Feb 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Rebased to a recent master:
@echesakovMSFT PTAL
set COMPlus_AltJitName=protononjit.dll
set COMPLUS_AltJitNgen=*
set COMPlus_TieredCompilation=0
crossgen.exe System.Private.CoreLib.dll
PerfScore: unhandled instruction: uminv, format IF_DV_2T
Assert failure(PID 53748 [0x0000d1f4], Thread: 55356 [0xd83c]): Assertion failed '!"PerfScore: unhandled instruction"' in 'System.Numerics.Plane:Equals(System.Numerics.Plane):bool:this' during 'Generate code' (IL size 132)

@echesakov
echesakov
commented
Feb 28, 2020

Copy link


Copy Markdown

Contributor
@briansull I will add perf info later today - or if you want you can do this
@briansull
briansull
commented
Feb 28, 2020

Copy link


Copy Markdown

ContributorAuthor
@echesakovMSFT

I will look into adding them
@briansull
briansull
commented
Feb 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Added PerfScore values for IF_DV_2T: // addv, saddlv, smaxv, sminv, uaddlv, umaxv, uminv
@echesakov
echesakov
commented
Feb 28, 2020

Copy link


Copy Markdown

Contributor
Added PerfScore values for IF_DV_2T: // addv, saddlv, smaxv, sminv, uaddlv, umaxv, uminv
Thank you, @briansull !
@briansull
briansull
commented
Feb 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Add PerfScore support for fcmeq, fcmge, fcmgt, fcmle, fcmlt, fcvtl2, fcvtn, fcvtn2, fabd
@jaredparjaredpar
mentioned this pull request
Mar 3, 2020

Closed

@briansull
briansull
merged commit c1e17e1
into
dotnet:masterMar 3, 2020
@tannergooding
tannergooding
commented
Mar 3, 2020

Copy link


Copy Markdown

Member
Thanks @briansull!
@ghostghost
locked as resolved and limited conversation to collaborators
Dec 11, 2020
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@tannergoodingtannergoodingtannergooding approved these changes

+2 more reviewers

@CarolEidtCarolEidtCarolEidt left review comments
@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.

7 participants

@briansull@tannergooding@echesakov@stephentoub@CarolEidt@BruceForstall@sandreenko

, '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
Enable assert in perfScoreUnhandledInstruction - #810
Merged
briansull merged 6 commits into
dotnet:masterfrom 
briansull:perfscore-assert
Mar 3, 2020
Merged
Enable assert in perfScoreUnhandledInstruction#810
briansull merged 6 commits into
dotnet:masterfrom 
briansull:perfscore-assert
Conversation
@briansull
@briansullbriansull
commented
Dec 12, 2019

Copy link


Copy Markdown

Contributor
perfScoreUnhandledInstruction will now assert in a DEBUG or CHECKED build when it encounters an unhanded instruction
@briansullbriansull
mentioned this pull request
Dec 12, 2019
 Merged

@briansull
briansull
commented
Dec 12, 2019

Copy link


Copy Markdown

ContributorAuthor
Related to PR #751
@briansull
briansull
commented
Dec 12, 2019

Copy link


Copy Markdown

ContributorAuthor
@tannergooding@BruceForstall @dotnet/jit-contrib PTAL
BruceForstall
BruceForstall
approved these changes
Dec 12, 2019
View reviewed changes
@BruceForstallBruceForstall
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Can you make sure to run outerloop and outerloop stress jobs? I wouldn't want this to cause new asserts to show up, especially as we aren't actively monitoring all the various pipelines yet.
@briansull
briansull
commented
Dec 12, 2019

Copy link


Copy Markdown

ContributorAuthor
I am leaving this PR open as I will be leaving for a vacation shortly.
@sandreenkosandreenko
added
the 
area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
label
Dec 18, 2019
@briansull
briansull
commented
Jan 17, 2020

Copy link


Copy Markdown

ContributorAuthor
@BruceForstall I have refreshed this
@briansull
briansull
commented
Jan 17, 2020

Copy link


Copy Markdown

ContributorAuthor
/AZP list
@azure-pipelines
azure-pipelinesBot
commented
Jan 17, 2020

Copy link


Copy Markdown


CI/CD Pipelines for this repository:
@briansull
briansull
commented
Jan 17, 2020

Copy link


Copy Markdown

ContributorAuthor
/AZP runtime-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Jan 17, 2020

Copy link


Copy Markdown


Command 'runtime-coreclr' is not supported by Azure Pipelines.

Supported commands
  • help:
    • Get descriptions, examples and documentation about supported commands
    • Example: help "command_name"
  • list:
    • List all pipelines for this repository using a comment.
    • Example: "list"
  • run:
    • Run all pipelines or specific pipelines for this repository using a comment. Use this command by itself to trigger all related pipelines, or specify specific pipelines to run.
    • Example: "run" or "run pipeline_name, pipeline_name, pipeline_name"
  • where:
    • Report back the Azure DevOps orgs that are related to this repository and org
    • Example: "where"


See additional documentation.
@briansull
briansull
commented
Jan 17, 2020

Copy link


Copy Markdown

ContributorAuthor
/AZP run runtime-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Jan 17, 2020

Copy link


Copy Markdown


Azure Pipelines could not run because the pipeline triggers exclude this branch/path.
@briansull
briansull
commented
Jan 17, 2020

Copy link


Copy Markdown

ContributorAuthor
There are some unhanded instructions for ARM64:
https://helix.dot.net/api/2019-06-17/jobs/25335df6-3701-444a-878f-9e5140003fcd/workitems/JIT.HardwareIntrinsics/console
Assert failure(PID 220 [0x000000dc], Thread: 220 [0x00dc]): Assertion failed '!"PerfScore: unhandled instruction"' in 'JIT.HardwareIntrinsics.Arm.SimpleBinaryOpTest__AbsoluteCompareGreaterThan_Vector128_Double:RunBasicScenario_UnsafeRead():this' (IL size 114)
@tannergooding
tannergooding
commented
Jan 17, 2020

Copy link


Copy Markdown

Member
CC. @EgorBo, @TamarChristinaArm, @CarolEidt
@echesakov
echesakov
commented
Jan 17, 2020•

edited

Loading

Copy link


Copy Markdown

Contributor
@tannergooding I believe you wanted to ping me instead of Egor Bogatov. I added these instructions and I can update their perf score
@tannergooding
tannergooding
commented
Jan 17, 2020

Copy link


Copy Markdown

Member
Yes, sorry. Auto-complete chose the wrong person 😄
CarolEidt
CarolEidt
reviewed
Jan 17, 2020
View reviewed changes
@CarolEidtCarolEidt
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

My preference would be not to enable this assert. I'm not in favor of asserts that appear only when dumping or disassembling.
Comment threadsrc/coreclr/src/jit/emit.cpp
Outdated

@CarolEidtCarolEidtJan 17, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I think that this should be deleted, and the above comment should be clarified to say that it asserts in a DEBUG build, and returns a default latency of 1 cycle otherwise.
@briansullbriansullJan 17, 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.

OK
@briansull
briansull
commented
Jan 17, 2020

Copy link


Copy Markdown

ContributorAuthor
People working on Arm64 instructions could just add this change-set to their enlistment to catch the missing instructions. I put this PR up because Tanner wanted to have this enabled.
@briansull
briansull
commented
Jan 17, 2020

Copy link


Copy Markdown

ContributorAuthor
The outer loop testing printed these two missing instructions on Arm64:
PerfScore: unhandled instruction: facgt, format IF_DV_3B
PerfScore: unhandled instruction: crc32b, format IF_DR_3A

@echesakovechesakov
mentioned this pull request
Jan 31, 2020
 Merged

@stephentoub
stephentoub
commented
Feb 17, 2020

Copy link


Copy Markdown

Member
@briansull, what's the status of this PR? Should it be closed? Merged? Thanks.
@briansull
briansull
commented
Feb 18, 2020

Copy link


Copy Markdown

ContributorAuthor
@tannergooding do you still want this assert enabled by default?
@tannergooding
tannergooding
commented
Feb 18, 2020

Copy link


Copy Markdown

Member
It would be good to get it enabled as otherwise it just represents an increasing debt we are building for ARM64 (as we continue to add more instructions for the HWIntrinsic work, etc).
At this point, it should just require adding the instructions which aren't already covered, correct? Perhaps that is something that @echesakovMSFT, @CarolEidt, or myself can pick up and finish.
@CarolEidt
CarolEidt
commented
Feb 18, 2020

Copy link


Copy Markdown

Contributor
I'm still of the opinion that we don't want asserts that fire only when dumping or disassembling. I would be OK with a COMPlus option that enables the assert.
@tannergooding
tannergooding
commented
Feb 18, 2020

Copy link


Copy Markdown

Member
I'm still of the opinion that we don't want asserts that fire only when dumping or disassembling. I would be OK with a COMPlus option that enables the assert.
IIRC, this fell out of #751 (comment) and only wasn't enabled due to parallel work from the HWIntrinsics.

This PR would just be enabling the same validation we already have for x64.
@CarolEidt
CarolEidt
commented
Feb 18, 2020

Copy link


Copy Markdown

Contributor
My concern is with enabling an assert that only fires when I generate a dump or disassembly. This is not something we regularly do across all of our tests, and so it doesn't really add coverage. It is more likely to trip up someone who is working on something unrelated. These asserts should either be optionally enabled in dumps and disassembly (and included in some kind of stress testing) or the perfscore should always be computed for each instruction in checked builds, even when not disassembling.
@briansull
briansull
commented
Feb 18, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
The PerfScore is always computed for Checked and Debug builds, not only when you generate a dump or disassembly.
size_t emitter::emitIssue1Instr(insGroup* ig, instrDesc* id, BYTE** dp)
{
_..._
#if defined(DEBUG) || defined(LATE_DISASM)
float insExeCost = insEvaluateExecutionCost(id);

@CarolEidt
CarolEidt
commented
Feb 19, 2020

Copy link


Copy Markdown

Contributor
The PerfScore is always computed for Checked and Debug builds.
I see - I'd missed that - in that case, I'm OK with this. Sorry for the confusion.
@briansull
perfScoreUnhandledInstruction will now assert in a DEBUG or CHECKED b…
54c1e90
…uild when it encounters an unhanded instruction
@briansull
briansull
commented
Feb 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Rebased to a recent master:
@echesakovMSFT PTAL
set COMPlus_AltJitName=protononjit.dll
set COMPLUS_AltJitNgen=*
set COMPlus_TieredCompilation=0
crossgen.exe System.Private.CoreLib.dll
PerfScore: unhandled instruction: uminv, format IF_DV_2T
Assert failure(PID 53748 [0x0000d1f4], Thread: 55356 [0xd83c]): Assertion failed '!"PerfScore: unhandled instruction"' in 'System.Numerics.Plane:Equals(System.Numerics.Plane):bool:this' during 'Generate code' (IL size 132)

@echesakov
echesakov
commented
Feb 28, 2020

Copy link


Copy Markdown

Contributor
@briansull I will add perf info later today - or if you want you can do this
@briansull
briansull
commented
Feb 28, 2020

Copy link


Copy Markdown

ContributorAuthor
@echesakovMSFT

I will look into adding them
@briansull
briansull
commented
Feb 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Added PerfScore values for IF_DV_2T: // addv, saddlv, smaxv, sminv, uaddlv, umaxv, uminv
@echesakov
echesakov
commented
Feb 28, 2020

Copy link


Copy Markdown

Contributor
Added PerfScore values for IF_DV_2T: // addv, saddlv, smaxv, sminv, uaddlv, umaxv, uminv
Thank you, @briansull !
@briansull
briansull
commented
Feb 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Add PerfScore support for fcmeq, fcmge, fcmgt, fcmle, fcmlt, fcvtl2, fcvtn, fcvtn2, fabd
@jaredparjaredpar
mentioned this pull request
Mar 3, 2020

Closed

@briansull
briansull
merged commit c1e17e1
into
dotnet:masterMar 3, 2020
@tannergooding
tannergooding
commented
Mar 3, 2020

Copy link


Copy Markdown

Member
Thanks @briansull!
@ghostghost
locked as resolved and limited conversation to collaborators
Dec 11, 2020
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@tannergoodingtannergoodingtannergooding approved these changes

+2 more reviewers

@CarolEidtCarolEidtCarolEidt left review comments
@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.

7 participants

@briansull@tannergooding@echesakov@stephentoub@CarolEidt@BruceForstall@sandreenko

, '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
Enable assert in perfScoreUnhandledInstruction - #810
Merged
briansull merged 6 commits into
dotnet:masterfrom 
briansull:perfscore-assert
Mar 3, 2020
Merged
Enable assert in perfScoreUnhandledInstruction#810
briansull merged 6 commits into
dotnet:masterfrom 
briansull:perfscore-assert
Conversation
@briansull
@briansullbriansull
commented
Dec 12, 2019

Copy link


Copy Markdown

Contributor
perfScoreUnhandledInstruction will now assert in a DEBUG or CHECKED build when it encounters an unhanded instruction
@briansullbriansull
mentioned this pull request
Dec 12, 2019
 Merged

@briansull
briansull
commented
Dec 12, 2019

Copy link


Copy Markdown

ContributorAuthor
Related to PR #751
@briansull
briansull
commented
Dec 12, 2019

Copy link


Copy Markdown

ContributorAuthor
@tannergooding@BruceForstall @dotnet/jit-contrib PTAL
BruceForstall
BruceForstall
approved these changes
Dec 12, 2019
View reviewed changes
@BruceForstallBruceForstall
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Can you make sure to run outerloop and outerloop stress jobs? I wouldn't want this to cause new asserts to show up, especially as we aren't actively monitoring all the various pipelines yet.
@briansull
briansull
commented
Dec 12, 2019

Copy link


Copy Markdown

ContributorAuthor
I am leaving this PR open as I will be leaving for a vacation shortly.
@sandreenkosandreenko
added
the 
area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
label
Dec 18, 2019
@briansull
briansull
commented
Jan 17, 2020

Copy link


Copy Markdown

ContributorAuthor
@BruceForstall I have refreshed this
@briansull
briansull
commented
Jan 17, 2020

Copy link


Copy Markdown

ContributorAuthor
/AZP list
@azure-pipelines
azure-pipelinesBot
commented
Jan 17, 2020

Copy link


Copy Markdown


CI/CD Pipelines for this repository:
@briansull
briansull
commented
Jan 17, 2020

Copy link


Copy Markdown

ContributorAuthor
/AZP runtime-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Jan 17, 2020

Copy link


Copy Markdown


Command 'runtime-coreclr' is not supported by Azure Pipelines.

Supported commands
  • help:
    • Get descriptions, examples and documentation about supported commands
    • Example: help "command_name"
  • list:
    • List all pipelines for this repository using a comment.
    • Example: "list"
  • run:
    • Run all pipelines or specific pipelines for this repository using a comment. Use this command by itself to trigger all related pipelines, or specify specific pipelines to run.
    • Example: "run" or "run pipeline_name, pipeline_name, pipeline_name"
  • where:
    • Report back the Azure DevOps orgs that are related to this repository and org
    • Example: "where"


See additional documentation.
@briansull
briansull
commented
Jan 17, 2020

Copy link


Copy Markdown

ContributorAuthor
/AZP run runtime-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Jan 17, 2020

Copy link


Copy Markdown


Azure Pipelines could not run because the pipeline triggers exclude this branch/path.
@briansull
briansull
commented
Jan 17, 2020

Copy link


Copy Markdown

ContributorAuthor
There are some unhanded instructions for ARM64:
https://helix.dot.net/api/2019-06-17/jobs/25335df6-3701-444a-878f-9e5140003fcd/workitems/JIT.HardwareIntrinsics/console
Assert failure(PID 220 [0x000000dc], Thread: 220 [0x00dc]): Assertion failed '!"PerfScore: unhandled instruction"' in 'JIT.HardwareIntrinsics.Arm.SimpleBinaryOpTest__AbsoluteCompareGreaterThan_Vector128_Double:RunBasicScenario_UnsafeRead():this' (IL size 114)
@tannergooding
tannergooding
commented
Jan 17, 2020

Copy link


Copy Markdown

Member
CC. @EgorBo, @TamarChristinaArm, @CarolEidt
@echesakov
echesakov
commented
Jan 17, 2020•

edited

Loading

Copy link


Copy Markdown

Contributor
@tannergooding I believe you wanted to ping me instead of Egor Bogatov. I added these instructions and I can update their perf score
@tannergooding
tannergooding
commented
Jan 17, 2020

Copy link


Copy Markdown

Member
Yes, sorry. Auto-complete chose the wrong person 😄
CarolEidt
CarolEidt
reviewed
Jan 17, 2020
View reviewed changes
@CarolEidtCarolEidt
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

My preference would be not to enable this assert. I'm not in favor of asserts that appear only when dumping or disassembling.
Comment threadsrc/coreclr/src/jit/emit.cpp
Outdated

@CarolEidtCarolEidtJan 17, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I think that this should be deleted, and the above comment should be clarified to say that it asserts in a DEBUG build, and returns a default latency of 1 cycle otherwise.
@briansullbriansullJan 17, 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.

OK
@briansull
briansull
commented
Jan 17, 2020

Copy link


Copy Markdown

ContributorAuthor
People working on Arm64 instructions could just add this change-set to their enlistment to catch the missing instructions. I put this PR up because Tanner wanted to have this enabled.
@briansull
briansull
commented
Jan 17, 2020

Copy link


Copy Markdown

ContributorAuthor
The outer loop testing printed these two missing instructions on Arm64:
PerfScore: unhandled instruction: facgt, format IF_DV_3B
PerfScore: unhandled instruction: crc32b, format IF_DR_3A

@echesakovechesakov
mentioned this pull request
Jan 31, 2020
 Merged

@stephentoub
stephentoub
commented
Feb 17, 2020

Copy link


Copy Markdown

Member
@briansull, what's the status of this PR? Should it be closed? Merged? Thanks.
@briansull
briansull
commented
Feb 18, 2020

Copy link


Copy Markdown

ContributorAuthor
@tannergooding do you still want this assert enabled by default?
@tannergooding
tannergooding
commented
Feb 18, 2020

Copy link


Copy Markdown

Member
It would be good to get it enabled as otherwise it just represents an increasing debt we are building for ARM64 (as we continue to add more instructions for the HWIntrinsic work, etc).
At this point, it should just require adding the instructions which aren't already covered, correct? Perhaps that is something that @echesakovMSFT, @CarolEidt, or myself can pick up and finish.
@CarolEidt
CarolEidt
commented
Feb 18, 2020

Copy link


Copy Markdown

Contributor
I'm still of the opinion that we don't want asserts that fire only when dumping or disassembling. I would be OK with a COMPlus option that enables the assert.
@tannergooding
tannergooding
commented
Feb 18, 2020

Copy link


Copy Markdown

Member
I'm still of the opinion that we don't want asserts that fire only when dumping or disassembling. I would be OK with a COMPlus option that enables the assert.
IIRC, this fell out of #751 (comment) and only wasn't enabled due to parallel work from the HWIntrinsics.

This PR would just be enabling the same validation we already have for x64.
@CarolEidt
CarolEidt
commented
Feb 18, 2020

Copy link


Copy Markdown

Contributor
My concern is with enabling an assert that only fires when I generate a dump or disassembly. This is not something we regularly do across all of our tests, and so it doesn't really add coverage. It is more likely to trip up someone who is working on something unrelated. These asserts should either be optionally enabled in dumps and disassembly (and included in some kind of stress testing) or the perfscore should always be computed for each instruction in checked builds, even when not disassembling.
@briansull
briansull
commented
Feb 18, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
The PerfScore is always computed for Checked and Debug builds, not only when you generate a dump or disassembly.
size_t emitter::emitIssue1Instr(insGroup* ig, instrDesc* id, BYTE** dp)
{
_..._
#if defined(DEBUG) || defined(LATE_DISASM)
float insExeCost = insEvaluateExecutionCost(id);

@CarolEidt
CarolEidt
commented
Feb 19, 2020

Copy link


Copy Markdown

Contributor
The PerfScore is always computed for Checked and Debug builds.
I see - I'd missed that - in that case, I'm OK with this. Sorry for the confusion.
@briansull
perfScoreUnhandledInstruction will now assert in a DEBUG or CHECKED b…
54c1e90
…uild when it encounters an unhanded instruction
@briansull
briansull
commented
Feb 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Rebased to a recent master:
@echesakovMSFT PTAL
set COMPlus_AltJitName=protononjit.dll
set COMPLUS_AltJitNgen=*
set COMPlus_TieredCompilation=0
crossgen.exe System.Private.CoreLib.dll
PerfScore: unhandled instruction: uminv, format IF_DV_2T
Assert failure(PID 53748 [0x0000d1f4], Thread: 55356 [0xd83c]): Assertion failed '!"PerfScore: unhandled instruction"' in 'System.Numerics.Plane:Equals(System.Numerics.Plane):bool:this' during 'Generate code' (IL size 132)

@echesakov
echesakov
commented
Feb 28, 2020

Copy link


Copy Markdown

Contributor
@briansull I will add perf info later today - or if you want you can do this
@briansull
briansull
commented
Feb 28, 2020

Copy link


Copy Markdown

ContributorAuthor
@echesakovMSFT

I will look into adding them
@briansull
briansull
commented
Feb 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Added PerfScore values for IF_DV_2T: // addv, saddlv, smaxv, sminv, uaddlv, umaxv, uminv
@echesakov
echesakov
commented
Feb 28, 2020

Copy link


Copy Markdown

Contributor
Added PerfScore values for IF_DV_2T: // addv, saddlv, smaxv, sminv, uaddlv, umaxv, uminv
Thank you, @briansull !
@briansull
briansull
commented
Feb 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Add PerfScore support for fcmeq, fcmge, fcmgt, fcmle, fcmlt, fcvtl2, fcvtn, fcvtn2, fabd
@jaredparjaredpar
mentioned this pull request
Mar 3, 2020

Closed

@briansull
briansull
merged commit c1e17e1
into
dotnet:masterMar 3, 2020
@tannergooding
tannergooding
commented
Mar 3, 2020

Copy link


Copy Markdown

Member
Thanks @briansull!
@ghostghost
locked as resolved and limited conversation to collaborators
Dec 11, 2020
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@tannergoodingtannergoodingtannergooding approved these changes

+2 more reviewers

@CarolEidtCarolEidtCarolEidt left review comments
@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.

7 participants

@briansull@tannergooding@echesakov@stephentoub@CarolEidt@BruceForstall@sandreenko

, '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
Enable assert in perfScoreUnhandledInstruction - #810
Merged
briansull merged 6 commits into
dotnet:masterfrom 
briansull:perfscore-assert
Mar 3, 2020
Merged
Enable assert in perfScoreUnhandledInstruction#810
briansull merged 6 commits into
dotnet:masterfrom 
briansull:perfscore-assert
Conversation
@briansull
@briansullbriansull
commented
Dec 12, 2019

Copy link


Copy Markdown

Contributor
perfScoreUnhandledInstruction will now assert in a DEBUG or CHECKED build when it encounters an unhanded instruction
@briansullbriansull
mentioned this pull request
Dec 12, 2019
 Merged

@briansull
briansull
commented
Dec 12, 2019

Copy link


Copy Markdown

ContributorAuthor
Related to PR #751
@briansull
briansull
commented
Dec 12, 2019

Copy link


Copy Markdown

ContributorAuthor
@tannergooding@BruceForstall @dotnet/jit-contrib PTAL
BruceForstall
BruceForstall
approved these changes
Dec 12, 2019
View reviewed changes
@BruceForstallBruceForstall
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Can you make sure to run outerloop and outerloop stress jobs? I wouldn't want this to cause new asserts to show up, especially as we aren't actively monitoring all the various pipelines yet.
@briansull
briansull
commented
Dec 12, 2019

Copy link


Copy Markdown

ContributorAuthor
I am leaving this PR open as I will be leaving for a vacation shortly.
@sandreenkosandreenko
added
the 
area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
label
Dec 18, 2019
@briansull
briansull
commented
Jan 17, 2020

Copy link


Copy Markdown

ContributorAuthor
@BruceForstall I have refreshed this
@briansull
briansull
commented
Jan 17, 2020

Copy link


Copy Markdown

ContributorAuthor
/AZP list
@azure-pipelines
azure-pipelinesBot
commented
Jan 17, 2020

Copy link


Copy Markdown


CI/CD Pipelines for this repository:
@briansull
briansull
commented
Jan 17, 2020

Copy link


Copy Markdown

ContributorAuthor
/AZP runtime-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Jan 17, 2020

Copy link


Copy Markdown


Command 'runtime-coreclr' is not supported by Azure Pipelines.

Supported commands
  • help:
    • Get descriptions, examples and documentation about supported commands
    • Example: help "command_name"
  • list:
    • List all pipelines for this repository using a comment.
    • Example: "list"
  • run:
    • Run all pipelines or specific pipelines for this repository using a comment. Use this command by itself to trigger all related pipelines, or specify specific pipelines to run.
    • Example: "run" or "run pipeline_name, pipeline_name, pipeline_name"
  • where:
    • Report back the Azure DevOps orgs that are related to this repository and org
    • Example: "where"


See additional documentation.
@briansull
briansull
commented
Jan 17, 2020

Copy link


Copy Markdown

ContributorAuthor
/AZP run runtime-coreclr outerloop
@azure-pipelines
azure-pipelinesBot
commented
Jan 17, 2020

Copy link


Copy Markdown


Azure Pipelines could not run because the pipeline triggers exclude this branch/path.
@briansull
briansull
commented
Jan 17, 2020

Copy link


Copy Markdown

ContributorAuthor
There are some unhanded instructions for ARM64:
https://helix.dot.net/api/2019-06-17/jobs/25335df6-3701-444a-878f-9e5140003fcd/workitems/JIT.HardwareIntrinsics/console
Assert failure(PID 220 [0x000000dc], Thread: 220 [0x00dc]): Assertion failed '!"PerfScore: unhandled instruction"' in 'JIT.HardwareIntrinsics.Arm.SimpleBinaryOpTest__AbsoluteCompareGreaterThan_Vector128_Double:RunBasicScenario_UnsafeRead():this' (IL size 114)
@tannergooding
tannergooding
commented
Jan 17, 2020

Copy link


Copy Markdown

Member
CC. @EgorBo, @TamarChristinaArm, @CarolEidt
@echesakov
echesakov
commented
Jan 17, 2020•

edited

Loading

Copy link


Copy Markdown

Contributor
@tannergooding I believe you wanted to ping me instead of Egor Bogatov. I added these instructions and I can update their perf score
@tannergooding
tannergooding
commented
Jan 17, 2020

Copy link


Copy Markdown

Member
Yes, sorry. Auto-complete chose the wrong person 😄
CarolEidt
CarolEidt
reviewed
Jan 17, 2020
View reviewed changes
@CarolEidtCarolEidt
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

My preference would be not to enable this assert. I'm not in favor of asserts that appear only when dumping or disassembling.
Comment threadsrc/coreclr/src/jit/emit.cpp
Outdated

@CarolEidtCarolEidtJan 17, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

I think that this should be deleted, and the above comment should be clarified to say that it asserts in a DEBUG build, and returns a default latency of 1 cycle otherwise.
@briansullbriansullJan 17, 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.

OK
@briansull
briansull
commented
Jan 17, 2020

Copy link


Copy Markdown

ContributorAuthor
People working on Arm64 instructions could just add this change-set to their enlistment to catch the missing instructions. I put this PR up because Tanner wanted to have this enabled.
@briansull
briansull
commented
Jan 17, 2020

Copy link


Copy Markdown

ContributorAuthor
The outer loop testing printed these two missing instructions on Arm64:
PerfScore: unhandled instruction: facgt, format IF_DV_3B
PerfScore: unhandled instruction: crc32b, format IF_DR_3A

@echesakovechesakov
mentioned this pull request
Jan 31, 2020
 Merged

@stephentoub
stephentoub
commented
Feb 17, 2020

Copy link


Copy Markdown

Member
@briansull, what's the status of this PR? Should it be closed? Merged? Thanks.
@briansull
briansull
commented
Feb 18, 2020

Copy link


Copy Markdown

ContributorAuthor
@tannergooding do you still want this assert enabled by default?
@tannergooding
tannergooding
commented
Feb 18, 2020

Copy link


Copy Markdown

Member
It would be good to get it enabled as otherwise it just represents an increasing debt we are building for ARM64 (as we continue to add more instructions for the HWIntrinsic work, etc).
At this point, it should just require adding the instructions which aren't already covered, correct? Perhaps that is something that @echesakovMSFT, @CarolEidt, or myself can pick up and finish.
@CarolEidt
CarolEidt
commented
Feb 18, 2020

Copy link


Copy Markdown

Contributor
I'm still of the opinion that we don't want asserts that fire only when dumping or disassembling. I would be OK with a COMPlus option that enables the assert.
@tannergooding
tannergooding
commented
Feb 18, 2020

Copy link


Copy Markdown

Member
I'm still of the opinion that we don't want asserts that fire only when dumping or disassembling. I would be OK with a COMPlus option that enables the assert.
IIRC, this fell out of #751 (comment) and only wasn't enabled due to parallel work from the HWIntrinsics.

This PR would just be enabling the same validation we already have for x64.
@CarolEidt
CarolEidt
commented
Feb 18, 2020

Copy link


Copy Markdown

Contributor
My concern is with enabling an assert that only fires when I generate a dump or disassembly. This is not something we regularly do across all of our tests, and so it doesn't really add coverage. It is more likely to trip up someone who is working on something unrelated. These asserts should either be optionally enabled in dumps and disassembly (and included in some kind of stress testing) or the perfscore should always be computed for each instruction in checked builds, even when not disassembling.
@briansull
briansull
commented
Feb 18, 2020•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
The PerfScore is always computed for Checked and Debug builds, not only when you generate a dump or disassembly.
size_t emitter::emitIssue1Instr(insGroup* ig, instrDesc* id, BYTE** dp)
{
_..._
#if defined(DEBUG) || defined(LATE_DISASM)
float insExeCost = insEvaluateExecutionCost(id);

@CarolEidt
CarolEidt
commented
Feb 19, 2020

Copy link


Copy Markdown

Contributor
The PerfScore is always computed for Checked and Debug builds.
I see - I'd missed that - in that case, I'm OK with this. Sorry for the confusion.
@briansull
perfScoreUnhandledInstruction will now assert in a DEBUG or CHECKED b…
54c1e90
…uild when it encounters an unhanded instruction
@briansull
briansull
commented
Feb 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Rebased to a recent master:
@echesakovMSFT PTAL
set COMPlus_AltJitName=protononjit.dll
set COMPLUS_AltJitNgen=*
set COMPlus_TieredCompilation=0
crossgen.exe System.Private.CoreLib.dll
PerfScore: unhandled instruction: uminv, format IF_DV_2T
Assert failure(PID 53748 [0x0000d1f4], Thread: 55356 [0xd83c]): Assertion failed '!"PerfScore: unhandled instruction"' in 'System.Numerics.Plane:Equals(System.Numerics.Plane):bool:this' during 'Generate code' (IL size 132)

@echesakov
echesakov
commented
Feb 28, 2020

Copy link


Copy Markdown

Contributor
@briansull I will add perf info later today - or if you want you can do this
@briansull
briansull
commented
Feb 28, 2020

Copy link


Copy Markdown

ContributorAuthor
@echesakovMSFT

I will look into adding them
@briansull
briansull
commented
Feb 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Added PerfScore values for IF_DV_2T: // addv, saddlv, smaxv, sminv, uaddlv, umaxv, uminv
@echesakov
echesakov
commented
Feb 28, 2020

Copy link


Copy Markdown

Contributor
Added PerfScore values for IF_DV_2T: // addv, saddlv, smaxv, sminv, uaddlv, umaxv, uminv
Thank you, @briansull !
@briansull
briansull
commented
Feb 28, 2020

Copy link


Copy Markdown

ContributorAuthor
Add PerfScore support for fcmeq, fcmge, fcmgt, fcmle, fcmlt, fcvtl2, fcvtn, fcvtn2, fabd
@jaredparjaredpar
mentioned this pull request
Mar 3, 2020

Closed

@briansull
briansull
merged commit c1e17e1
into
dotnet:masterMar 3, 2020
@tannergooding
tannergooding
commented
Mar 3, 2020

Copy link


Copy Markdown

Member
Thanks @briansull!
@ghostghost
locked as resolved and limited conversation to collaborators
Dec 11, 2020
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@tannergoodingtannergoodingtannergooding approved these changes

+2 more reviewers

@CarolEidtCarolEidtCarolEidt left review comments
@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.

7 participants

@briansull@tannergooding@echesakov@stephentoub@CarolEidt@BruceForstall@sandreenko