Skip to content

JIT: Enabled embedded broadcast for binary ops - #87946

Merged
tannergooding merged 9 commits into
dotnet:mainfrom
Ruihan-Yin:EbBinary
Jul 14, 2023
Merged

JIT: Enabled embedded broadcast for binary ops#87946
tannergooding merged 9 commits into
dotnet:mainfrom
Ruihan-Yin:EbBinary

Conversation

@Ruihan-Yin

@Ruihan-YinRuihan-Yin commented Jun 22, 2023

Copy link
Copy Markdown
Member

This PR provides the embedded broadcast support for binary ops, to be more specific, ops using the genHWIntrinsic_R_R_RM path.

Including the following ops:
and, andn, or, xor,
min, max,
div, mul, mull, sub,
variable shiftleftlogical/rightarithmetic/rightlogical

@ghostghost added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI community-contribution Indicates that the PR has been added by a community member labels Jun 22, 2023
@ghost

ghost commented Jun 22, 2023

Copy link
Copy Markdown

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

Issue Details

This PR provides the embedded broadcast support for binary ops, to be more specific, ops using the genHWIntrinsic_R_R_RM path.

Including the following ops:
and, andn, or, xor,
min, max,
div, mul, mull, sub,
variable shiftleftlogical/rightarithmetic/rightlogical

Author:Ruihan-Yin
Assignees:-
Labels:

area-CodeGen-coreclr

Milestone:-

@build-analysisbuild-analysisBot mentioned this pull request Jun 23, 2023
Closed
@Ruihan-YinRuihan-Yin changed the title Enabled embedded broadcast for binary opsJIT: Enabled embedded broadcast for binary opsJun 23, 2023
@Ruihan-YinRuihan-Yin reopened this Jun 23, 2023
@tannergoodingtannergooding added the avx512 Related to the AVX-512 architecture label Jun 27, 2023
@Ruihan-Yin
Ruihan-Yin marked this pull request as ready for review June 27, 2023 22:01
@Ruihan-Yin

Ruihan-Yin commented Jun 28, 2023•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

Hi @tannergooding , the PR should be ready for review, would you please take a look at it?
it mostly includes:

  1. some updates on the metadata in the related tables to enable embedded broadcast in more instructions.
  2. some workaround on the bitwise instructions to make it work correctly with embedded broadcast when the input data type is in 64-bit.
  3. a bug fix where some broadcast nodes with data type less than 32 bits are accidentally contained, made some changes in the contain check to filter out these situations.

@build-analysisbuild-analysisBot mentioned this pull request Jun 29, 2023
Closed
tannergooding
tannergooding reviewed Jun 29, 2023
View reviewed changes
Comment threadsrc/coreclr/jit/instr.cpp Outdated
@build-analysisbuild-analysisBot mentioned this pull request Jun 29, 2023
Closed
@Ruihan-Yin

Ruihan-Yin commented Jun 29, 2023

Copy link
Copy Markdown
MemberAuthor

windows x64

Diffs are based on 1,627,004 contexts (467,427 MinOpts, 1,159,577 FullOpts).

MISSED contexts: 2,227 (0.14%)

Overall (+2,406 bytes)
CollectionBase size (bytes)Diff size (bytes)
aspnet.run.windows.x64.checked.mch44,062,516+156
benchmarks.run.windows.x64.checked.mch7,244,558+152
benchmarks.run_pgo.windows.x64.checked.mch26,963,808+143
benchmarks.run_tiered.windows.x64.checked.mch10,507,436+157
coreclr_tests.run.windows.x64.checked.mch385,009,583+736
libraries.pmi.windows.x64.checked.mch59,506,418+190
libraries_tests.pmi.windows.x64.checked.mch121,311,207+220
realworld.run.windows.x64.checked.mch13,650,175+652
MinOpts (+383 bytes)
CollectionBase size (bytes)Diff size (bytes)
benchmarks.run_pgo.windows.x64.checked.mch10,473,053+73
benchmarks.run_tiered.windows.x64.checked.mch7,567,856+73
coreclr_tests.run.windows.x64.checked.mch269,424,110+237
FullOpts (+2,023 bytes)
CollectionBase size (bytes)Diff size (bytes)
aspnet.run.windows.x64.checked.mch29,278,684+156
benchmarks.run.windows.x64.checked.mch6,870,267+152
benchmarks.run_pgo.windows.x64.checked.mch16,490,755+70
benchmarks.run_tiered.windows.x64.checked.mch2,939,580+84
coreclr_tests.run.windows.x64.checked.mch115,585,473+499
libraries.pmi.windows.x64.checked.mch57,987,107+190
libraries_tests.pmi.windows.x64.checked.mch114,958,091+220
realworld.run.windows.x64.checked.mch12,500,933+652

@Ruihan-Yin

Ruihan-Yin commented Jun 29, 2023•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

Fails should be irrelevant.

We had regression of about 2,400 bytes in the code size introduced by the changes. This should be expected as the regression is the conversion from VEX encoding to EVEX encoding when enabling embedded broadcast.
And there is no influence on the throughput.

Do we accept the results?

@tannergooding

tannergooding commented Jul 6, 2023

Copy link
Copy Markdown
Member

Do we accept the results?

The results look good/within reason to me. There are two important bits to call out...

First this change, in the typical case, trades a slight increase in code size (typically +2-bytes) for a decrease in data size (going from sizeof(VectorXXX<T>) to sizeof(T)) thereby increasing data locality and minimizing impact to the cache. This will often be a performance win.

Second, SPMI diffs are not currently tracking/including the data size allocations as part of "bytes of code" metric. HardwareIntrinsics.RayTracer.Packet256Tracer:Shade for example is +2 bytes of code, but -32 bytes of data so its actually a net win of -30 bytes of allocations

tannergooding
tannergooding reviewed Jul 6, 2023
View reviewed changes
Comment threadsrc/coreclr/jit/lowerxarch.cpp Outdated

@tannergoodingtannergoodingJul 6, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is better handled as

Suggested change
if (baseType == TYP_BYTE || baseType == TYP_UBYTE || baseType == TYP_SHORT || baseType == TYP_USHORT)
if (varTypeIsSmall(baseType))

@tannergooding

tannergooding commented Jul 6, 2023

Copy link
Copy Markdown
Member

CC. @dotnet/jit-contrib for secondary review.

@BruceForstall

BruceForstall commented Jul 6, 2023

Copy link
Copy Markdown
Contributor

superpmi-replay and superpmi-diffs pipelines are failing with messages like:

[17:40:21] ERROR: Couldn't load base metrics summary created by child process
[17:40:21] General fatal error

I wonder if the JIT is crashing (not asserting) with this PR?

@Ruihan-Yin

Ruihan-Yin commented Jul 6, 2023

Copy link
Copy Markdown
MemberAuthor

I wonder if the JIT is crashing (not asserting) with this PR?

First time seeing this fail in this PR, the recent changes might not cause the crashing, I will rebase the PR and re-run the CI again to confirm.

BruceForstall
BruceForstall reviewed Jul 7, 2023
View reviewed changes
Comment threadsrc/coreclr/jit/hwintrinsic.h Outdated

@BruceForstallBruceForstallJul 6, 2023

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.

This seems like an odd location for this function. All the other functions here take a NamedIntrinsic. Should this instead be in emitxarch.h/cpp like IsMovInstruction or HasKMaskRegisterDest ?

@Ruihan-YinRuihan-YinJul 10, 2023

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

moved the function into emitxarch.h/cpp as a static function.

Comment threadsrc/coreclr/jit/instrsxarch.h Outdated

@BruceForstallBruceForstallJul 7, 2023

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.

nit: no need to delete this empty line?

@tannergooding

tannergooding commented Jul 7, 2023

Copy link
Copy Markdown
Member

I wonder if the JIT is crashing (not asserting) with this PR?

There are similar failures in multiple PRs from what I've seen recently.

Noting that SPMI diffs is faulting in Python when processing the diffs summary:

Traceback (most recent call last):
File "C:\h\w\B2080989\p\superpmi.py", line 4667, in <module>
sys.exit(main(args))
File "C:\h\w\B2080989\p\superpmi.py", line 4558, in main
success = asm_diffs.replay_with_asm_diffs()
File "C:\h\w\B2080989\p\superpmi.py", line 2019, in replay_with_asm_diffs
self.write_asmdiffs_markdown_summary(write_fh, asm_diffs)
File "C:\h\w\B2080989\p\superpmi.py", line 2170, in write_asmdiffs_markdown_summary
write_row(*t)
File "C:\h\w\B2080989\p\superpmi.py", line 2165, in write_row
num_missed_base / num_contexts * 100,
ZeroDivisionError: division by zero

Replay is similarly failing with things like:

[17:42:50] Invoking: C:\h\w\B6DA095E\p\superpmi.exe -v ewi -r C:\h\w\B6DA095E\t\tmpdsiibugo\repro -p -jitoption JitStressRegs=0x80 -f C:\h\w\B6DA095E\t\tmpdsiibugo\coreclr_tests.run.windows.x64.checked.mch_fail.mcl -metricsSummary C:\h\w\B6DA095E\t\tmpdsiibugo\coreclr_tests.run.windows.x64.checked.mch_metrics.csv C:\h\w\B6DA095E\p\clrjit_win_x64_x64.dll C:\h\w\B6DA095E\p\artifacts\spmi\mch\02e334af-4e6e-4a68-9feb-308d3d2661bc.windows.x64\coreclr_tests.run.windows.x64.checked.mch
[17:42:50] ERROR: Couldn't load base metrics summary created by child process
[17:42:50] ERROR: Couldn't load base metrics summary created by child process
[17:42:50] ERROR: Couldn't load base metrics summary created by child process
[17:42:50] ERROR: Couldn't load base metrics summary created by child process
[17:42:50] General fatal error
[17:42:50] Running SuperPMI replay of C:\h\w\B6DA095E\p\artifacts\spmi\mch\02e334af-4e6e-4a68-9feb-308d3d2661bc.windows.x64\libraries.crossgen2.windows.x64.checked.mch

Did we recently get a JIT/EE version change and potentially not have up to date baselines?

@BruceForstall

BruceForstall commented Jul 7, 2023

Copy link
Copy Markdown
Contributor

Noting that SPMI diffs is faulting in Python when processing the diffs summary:

superpmi.py should gracefully handle that. But I think that's a symptom that superpmi.exe crashed.

Did we recently get a JIT/EE version change and potentially not have up to date baselines?

If the GUID changed, we wouldn't have this problem because we would only pick up matched MCH files.

If someone changed the interface without changing the GUID, then anything is possible.

@BruceForstall

BruceForstall commented Jul 7, 2023

Copy link
Copy Markdown
Contributor

I kicked off runs on main (not on a PR) so we can see if there are failures in the main branch:

https://dev.azure.com/dnceng-public/public/_build/results?buildId=331428&view=results
https://dev.azure.com/dnceng-public/public/_build/results?buildId=331429&view=results

@BruceForstall

BruceForstall commented Jul 7, 2023

Copy link
Copy Markdown
Contributor

@Ruihan-Yin The superpmi pipelines are now running clean. I would suggest to merge up to HEAD and re-push your PR to trigger re-run of these pipelines.

@Ruihan-Yin

Ruihan-Yin commented Jul 7, 2023•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

The superpmi pipelines are now running clean. I would suggest to merge up to HEAD and re-push your PR to trigger re-run of these pipelines.

Thanks for the notification, will resolve the reviews and rebase the branch soon.

@Ruihan-Yin
Enabled embedded broadcast for the following ops:
5ed01d7
and, andn, or, xor,
min, max,
div, mul, mull, sub,
variable shiftleftlogical/rightarithmetic/rightlogical
@Ruihan-Yin
Bug fix:
0f746c4
 JIT used to use a uniform intrinsic for bitwise operations with all data
types, embedded broadcast is sensitive to input size in this case,
adding a helper to let emitter aware when input size is long/ulong.
@Ruihan-Yin
reset the instruction in the later phase
7825a18
when embedded broadcast is actually enabled
@Ruihan-Yin
filter irrelevant data type in embedded broadcast
1513e55
There are cases when broadcast node are falsely contained by a embedded
broadcast compatible node, while the data type is actually not supported
Adding extra logics to avoid this situation.
@Ruihan-Yin
update the condition on instruction reset:
a326e58
instructions with either long or ulong as basetype should be reset to
qword instructions.
@Ruihan-Yin
Apply format patch
4468b9f
@Ruihan-Yin
Resolve reviews:
89120d2
make the typecheck based on broadcast node it self.
@Ruihan-Yin
Resolve review:
f31da65
use `varTypeIsSmall` type check to cover all the unsupported data type
in embedded broadcast.
@Ruihan-Yin
Resolve reviews:
23c3f6e
1. put the IsBitwiseInstruction to a proper place.
2. nit: restored unnecessary line delete.
@Ruihan-Yin

Ruihan-Yin commented Jul 10, 2023

Copy link
Copy Markdown
MemberAuthor

Hi @BruceForstall, some tests are cancelled for some reasons, I suppose it is not relevant to the changes in this PR, shall I re-run the test again? And can you please review the new changes, if any. Thanks!

EgorBo
EgorBo reviewed Jul 14, 2023
View reviewed changes
Comment threadsrc/coreclr/jit/instr.cpp
if (IsEmbBroadcast)
{
instOptions = INS_OPTS_EVEX_b;
if (emitter::IsBitwiseInstruction(ins) && varTypeIsLong(op2->AsHWIntrinsic()->GetSimdBaseType()))

@EgorBoEgorBoJul 14, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: do we need IsBitwiseInstruction at all since we have a switch over ins anyway? (we can just remove the unreached in debug

@tannergoodingtannergoodingJul 14, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Possibly not. I'll let Ruihan-Yin fix this in a follow up PR if they want to, that way we can land the important bit before the snap.

@Ruihan-YinRuihan-YinJul 14, 2023

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Thanks for the feedback, we will fix this point in the following PR on embedded broadcast.

@tannergooding
tannergooding merged commit ef9a07c into dotnet:mainJul 14, 2023
@JulieLeeMSFTJulieLeeMSFT mentioned this pull request Aug 2, 2023
Closed
3 tasks
@BruceForstallBruceForstall mentioned this pull request Aug 3, 2023
Merged
@ghostghost locked as resolved and limited conversation to collaborators Aug 14, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@tannergoodingtannergoodingtannergooding approved these changes
@EgorBoEgorBoEgorBo approved these changes
@BruceForstallBruceForstallAwaiting requested review from BruceForstall

Assignees

No one assigned

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMIavx512Related to the AVX-512 architecturecommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Ruihan-Yin@tannergooding@BruceForstall@EgorBo
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all 
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
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;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
JIT: Enabled embedded broadcast for binary ops by Ruihan-Yin · Pull Request #87946 · dotnet/runtime · GitHub
Skip to content
JIT: Enabled embedded broadcast for binary ops - #87946
Merged
tannergooding merged 9 commits into
dotnet:mainfrom 
Ruihan-Yin:EbBinary
Jul 14, 2023
Merged
JIT: Enabled embedded broadcast for binary ops#87946
tannergooding merged 9 commits into
dotnet:mainfrom 
Ruihan-Yin:EbBinary
Conversation
@Ruihan-Yin
@Ruihan-YinRuihan-Yin
commented
Jun 22, 2023

Copy link


Copy Markdown

Member
This PR provides the embedded broadcast support for binary ops, to be more specific, ops using the genHWIntrinsic_R_R_RM path.
Including the following ops:

and, andn, or, xor,

min, max,

div, mul, mull, sub,

variable shiftleftlogical/rightarithmetic/rightlogical
@ghostghost
added

area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
community-contribution
Indicates that the PR has been added by a community member
labels
Jun 22, 2023
@ghost
ghost
commented
Jun 22, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch

See info in area-owners.md if you want to be subscribed.
Issue Details
This PR provides the embedded broadcast support for binary ops, to be more specific, ops using the genHWIntrinsic_R_R_RM path.
Including the following ops:

and, andn, or, xor,

min, max,

div, mul, mull, sub,

variable shiftleftlogical/rightarithmetic/rightlogical
Author:Ruihan-Yin
Assignees:-
Labels:
area-CodeGen-coreclr
Milestone:-
@build-analysisbuild-analysisBot
mentioned this pull request
Jun 23, 2023

Closed

@Ruihan-YinRuihan-Yin
changed the title
Enabled embedded broadcast for binary opsJIT: Enabled embedded broadcast for binary opsJun 23, 2023
@Ruihan-YinRuihan-Yin
reopened this
Jun 23, 2023
@tannergoodingtannergooding
added
the 
avx512
Related to the AVX-512 architecture
label
Jun 27, 2023
@Ruihan-Yin
Ruihan-Yin
marked this pull request as ready for review
June 27, 2023 22:01
@Ruihan-Yin
Ruihan-Yin
commented
Jun 28, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Hi @tannergooding , the PR should be ready for review, would you please take a look at it?

it mostly includes:
  1. some updates on the metadata in the related tables to enable embedded broadcast in more instructions.
  2. some workaround on the bitwise instructions to make it work correctly with embedded broadcast when the input data type is in 64-bit.
  3. a bug fix where some broadcast nodes with data type less than 32 bits are accidentally contained, made some changes in the contain check to filter out these situations.
@build-analysisbuild-analysisBot
mentioned this pull request
Jun 29, 2023

Closed

tannergooding
tannergooding
reviewed
Jun 29, 2023
View reviewed changes
Comment threadsrc/coreclr/jit/instr.cpp
Outdated

@build-analysisbuild-analysisBot
mentioned this pull request
Jun 29, 2023

Closed

@Ruihan-Yin
Ruihan-Yin
commented
Jun 29, 2023

Copy link


Copy Markdown

MemberAuthor
windows x64
Diffs are based on 1,627,004 contexts (467,427 MinOpts, 1,159,577 FullOpts).
MISSED contexts: 2,227 (0.14%)
Overall (+2,406 bytes)
CollectionBase size (bytes)Diff size (bytes)
aspnet.run.windows.x64.checked.mch44,062,516+156
benchmarks.run.windows.x64.checked.mch7,244,558+152
benchmarks.run_pgo.windows.x64.checked.mch26,963,808+143
benchmarks.run_tiered.windows.x64.checked.mch10,507,436+157
coreclr_tests.run.windows.x64.checked.mch385,009,583+736
libraries.pmi.windows.x64.checked.mch59,506,418+190
libraries_tests.pmi.windows.x64.checked.mch121,311,207+220
realworld.run.windows.x64.checked.mch13,650,175+652
MinOpts (+383 bytes)
CollectionBase size (bytes)Diff size (bytes)
benchmarks.run_pgo.windows.x64.checked.mch10,473,053+73
benchmarks.run_tiered.windows.x64.checked.mch7,567,856+73
coreclr_tests.run.windows.x64.checked.mch269,424,110+237
FullOpts (+2,023 bytes)
CollectionBase size (bytes)Diff size (bytes)
aspnet.run.windows.x64.checked.mch29,278,684+156
benchmarks.run.windows.x64.checked.mch6,870,267+152
benchmarks.run_pgo.windows.x64.checked.mch16,490,755+70
benchmarks.run_tiered.windows.x64.checked.mch2,939,580+84
coreclr_tests.run.windows.x64.checked.mch115,585,473+499
libraries.pmi.windows.x64.checked.mch57,987,107+190
libraries_tests.pmi.windows.x64.checked.mch114,958,091+220
realworld.run.windows.x64.checked.mch12,500,933+652
@Ruihan-Yin
Ruihan-Yin
commented
Jun 29, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Fails should be irrelevant.
We had regression of about 2,400 bytes in the code size introduced by the changes. This should be expected as the regression is the conversion from VEX encoding to EVEX encoding when enabling embedded broadcast.

And there is no influence on the throughput.
Do we accept the results?
@tannergooding
tannergooding
commented
Jul 6, 2023

Copy link


Copy Markdown

Member
Do we accept the results?
The results look good/within reason to me. There are two important bits to call out...
First this change, in the typical case, trades a slight increase in code size (typically +2-bytes) for a decrease in data size (going from sizeof(VectorXXX<T>) to sizeof(T)) thereby increasing data locality and minimizing impact to the cache. This will often be a performance win.
Second, SPMI diffs are not currently tracking/including the data size allocations as part of "bytes of code" metric. HardwareIntrinsics.RayTracer.Packet256Tracer:Shade for example is +2 bytes of code, but -32 bytes of data so its actually a net win of -30 bytes of allocations
tannergooding
tannergooding
reviewed
Jul 6, 2023
View reviewed changes
Comment threadsrc/coreclr/jit/lowerxarch.cpp
Outdated

@tannergoodingtannergoodingJul 6, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This is better handled as

Suggested change

if (baseType == TYP_BYTE || baseType == TYP_UBYTE || baseType == TYP_SHORT || baseType == TYP_USHORT)
if (varTypeIsSmall(baseType))
@tannergooding
tannergooding
commented
Jul 6, 2023

Copy link


Copy Markdown

Member
CC. @dotnet/jit-contrib for secondary review.
@BruceForstall
BruceForstall
commented
Jul 6, 2023

Copy link


Copy Markdown

Contributor
superpmi-replay and superpmi-diffs pipelines are failing with messages like:
[17:40:21] ERROR: Couldn't load base metrics summary created by child process
[17:40:21] General fatal error

I wonder if the JIT is crashing (not asserting) with this PR?
@Ruihan-Yin
Ruihan-Yin
commented
Jul 6, 2023

Copy link


Copy Markdown

MemberAuthor
I wonder if the JIT is crashing (not asserting) with this PR?
First time seeing this fail in this PR, the recent changes might not cause the crashing, I will rebase the PR and re-run the CI again to confirm.
BruceForstall
BruceForstall
reviewed
Jul 7, 2023
View reviewed changes
Comment threadsrc/coreclr/jit/hwintrinsic.h
Outdated

@BruceForstallBruceForstallJul 6, 2023

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.

This seems like an odd location for this function. All the other functions here take a NamedIntrinsic. Should this instead be in emitxarch.h/cpp like IsMovInstruction or HasKMaskRegisterDest ?
@Ruihan-YinRuihan-YinJul 10, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

moved the function into emitxarch.h/cpp as a static function.
Comment threadsrc/coreclr/jit/instrsxarch.h
Outdated

@BruceForstallBruceForstallJul 7, 2023

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.

nit: no need to delete this empty line?
@tannergooding
tannergooding
commented
Jul 7, 2023

Copy link


Copy Markdown

Member
I wonder if the JIT is crashing (not asserting) with this PR?
There are similar failures in multiple PRs from what I've seen recently.
Noting that SPMI diffs is faulting in Python when processing the diffs summary:
Traceback (most recent call last):
File "C:\h\w\B2080989\p\superpmi.py", line 4667, in <module>
sys.exit(main(args))
File "C:\h\w\B2080989\p\superpmi.py", line 4558, in main
success = asm_diffs.replay_with_asm_diffs()
File "C:\h\w\B2080989\p\superpmi.py", line 2019, in replay_with_asm_diffs
self.write_asmdiffs_markdown_summary(write_fh, asm_diffs)
File "C:\h\w\B2080989\p\superpmi.py", line 2170, in write_asmdiffs_markdown_summary
write_row(*t)
File "C:\h\w\B2080989\p\superpmi.py", line 2165, in write_row
num_missed_base / num_contexts * 100,
ZeroDivisionError: division by zero

Replay is similarly failing with things like:
[17:42:50] Invoking: C:\h\w\B6DA095E\p\superpmi.exe -v ewi -r C:\h\w\B6DA095E\t\tmpdsiibugo\repro -p -jitoption JitStressRegs=0x80 -f C:\h\w\B6DA095E\t\tmpdsiibugo\coreclr_tests.run.windows.x64.checked.mch_fail.mcl -metricsSummary C:\h\w\B6DA095E\t\tmpdsiibugo\coreclr_tests.run.windows.x64.checked.mch_metrics.csv C:\h\w\B6DA095E\p\clrjit_win_x64_x64.dll C:\h\w\B6DA095E\p\artifacts\spmi\mch\02e334af-4e6e-4a68-9feb-308d3d2661bc.windows.x64\coreclr_tests.run.windows.x64.checked.mch
[17:42:50] ERROR: Couldn't load base metrics summary created by child process
[17:42:50] ERROR: Couldn't load base metrics summary created by child process
[17:42:50] ERROR: Couldn't load base metrics summary created by child process
[17:42:50] ERROR: Couldn't load base metrics summary created by child process
[17:42:50] General fatal error
[17:42:50] Running SuperPMI replay of C:\h\w\B6DA095E\p\artifacts\spmi\mch\02e334af-4e6e-4a68-9feb-308d3d2661bc.windows.x64\libraries.crossgen2.windows.x64.checked.mch

Did we recently get a JIT/EE version change and potentially not have up to date baselines?
@BruceForstall
BruceForstall
commented
Jul 7, 2023

Copy link


Copy Markdown

Contributor
Noting that SPMI diffs is faulting in Python when processing the diffs summary:
superpmi.py should gracefully handle that. But I think that's a symptom that superpmi.exe crashed.
Did we recently get a JIT/EE version change and potentially not have up to date baselines?
If the GUID changed, we wouldn't have this problem because we would only pick up matched MCH files.
If someone changed the interface without changing the GUID, then anything is possible.
@BruceForstall
BruceForstall
commented
Jul 7, 2023

Copy link


Copy Markdown

Contributor
I kicked off runs on main (not on a PR) so we can see if there are failures in the main branch:
https://dev.azure.com/dnceng-public/public/_build/results?buildId=331428&view=results
https://dev.azure.com/dnceng-public/public/_build/results?buildId=331429&view=results
@BruceForstall
BruceForstall
commented
Jul 7, 2023

Copy link


Copy Markdown

Contributor
@Ruihan-Yin The superpmi pipelines are now running clean. I would suggest to merge up to HEAD and re-push your PR to trigger re-run of these pipelines.
@Ruihan-Yin
Ruihan-Yin
commented
Jul 7, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
The superpmi pipelines are now running clean. I would suggest to merge up to HEAD and re-push your PR to trigger re-run of these pipelines.
Thanks for the notification, will resolve the reviews and rebase the branch soon.
@Ruihan-Yin
Enabled embedded broadcast for the following ops:
5ed01d7
and, andn, or, xor,
min, max,
div, mul, mull, sub,
variable shiftleftlogical/rightarithmetic/rightlogical
@Ruihan-Yin
 Bug fix:
0f746c4
 JIT used to use a uniform intrinsic for bitwise operations with all data
types, embedded broadcast is sensitive to input size in this case,
adding a helper to let emitter aware when input size is long/ulong.
@Ruihan-Yin
reset the instruction in the later phase
7825a18
when embedded broadcast is actually enabled
@Ruihan-Yin
filter irrelevant data type in embedded broadcast
1513e55
There are cases when broadcast node are falsely contained by a embedded
broadcast compatible node, while the data type is actually not supported
Adding extra logics to avoid this situation.
@Ruihan-Yin
update the condition on instruction reset:
a326e58
instructions with either long or ulong as basetype should be reset to
qword instructions.
@Ruihan-Yin
Apply format patch
4468b9f
@Ruihan-Yin
Resolve reviews:
89120d2
make the typecheck based on broadcast node it self.
@Ruihan-Yin
Resolve review:
f31da65
use `varTypeIsSmall` type check to cover all the unsupported data type
in embedded broadcast.
@Ruihan-Yin
Resolve reviews:
23c3f6e
1. put the IsBitwiseInstruction to a proper place.
2. nit: restored unnecessary line delete.
@Ruihan-Yin
Ruihan-Yin
commented
Jul 10, 2023

Copy link


Copy Markdown

MemberAuthor
Hi @BruceForstall, some tests are cancelled for some reasons, I suppose it is not relevant to the changes in this PR, shall I re-run the test again? And can you please review the new changes, if any. Thanks!
EgorBo
EgorBo
reviewed
Jul 14, 2023
View reviewed changes
Comment threadsrc/coreclr/jit/instr.cpp
 if (IsEmbBroadcast)
 {
 instOptions = INS_OPTS_EVEX_b;
 if (emitter::IsBitwiseInstruction(ins) && varTypeIsLong(op2->AsHWIntrinsic()->GetSimdBaseType()))
@EgorBoEgorBoJul 14, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

nit: do we need IsBitwiseInstruction at all since we have a switch over ins anyway? (we can just remove the unreached in debug
@tannergoodingtannergoodingJul 14, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Possibly not. I'll let Ruihan-Yin fix this in a follow up PR if they want to, that way we can land the important bit before the snap.
@Ruihan-YinRuihan-YinJul 14, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Thanks for the feedback, we will fix this point in the following PR on embedded broadcast.
@tannergooding
tannergooding
merged commit ef9a07c
into
dotnet:mainJul 14, 2023
@JulieLeeMSFTJulieLeeMSFT
mentioned this pull request
Aug 2, 2023

Closed

3 tasks
@BruceForstallBruceForstall
mentioned this pull request
Aug 3, 2023
 Merged

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


Reviewers

@tannergoodingtannergoodingtannergooding approved these changes
@EgorBoEgorBoEgorBo approved these changes
@BruceForstallBruceForstallAwaiting requested review from BruceForstall

Assignees


No one assigned


Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMIavx512Related to the AVX-512 architecturecommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@Ruihan-Yin@tannergooding@BruceForstall@EgorBo

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Force GitHub README to respect dark mode
(function() {
var style = document.createElement('style');
style.textContent = '
.markdown-body {
color-scheme: dark light;
}
.markdown-body pre { background: #161b22 !important; }
.markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }
.markdown-body table th, .markdown-body table td { border-color: #30363d !important; }
.markdown-body img { background: #0d1117; }
.markdown-body blockquote { border-left-color: #8b949e; }
.markdown-body hr { border-color: #30363d; }
';
document.head.appendChild(style);
})();
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
JIT: Enabled embedded broadcast for binary ops by Ruihan-Yin · Pull Request #87946 · dotnet/runtime · GitHub
Skip to content
JIT: Enabled embedded broadcast for binary ops - #87946
Merged
tannergooding merged 9 commits into
dotnet:mainfrom 
Ruihan-Yin:EbBinary
Jul 14, 2023
Merged
JIT: Enabled embedded broadcast for binary ops#87946
tannergooding merged 9 commits into
dotnet:mainfrom 
Ruihan-Yin:EbBinary
Conversation
@Ruihan-Yin
@Ruihan-YinRuihan-Yin
commented
Jun 22, 2023

Copy link


Copy Markdown

Member
This PR provides the embedded broadcast support for binary ops, to be more specific, ops using the genHWIntrinsic_R_R_RM path.
Including the following ops:

and, andn, or, xor,

min, max,

div, mul, mull, sub,

variable shiftleftlogical/rightarithmetic/rightlogical
@ghostghost
added

area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
community-contribution
Indicates that the PR has been added by a community member
labels
Jun 22, 2023
@ghost
ghost
commented
Jun 22, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch

See info in area-owners.md if you want to be subscribed.
Issue Details
This PR provides the embedded broadcast support for binary ops, to be more specific, ops using the genHWIntrinsic_R_R_RM path.
Including the following ops:

and, andn, or, xor,

min, max,

div, mul, mull, sub,

variable shiftleftlogical/rightarithmetic/rightlogical
Author:Ruihan-Yin
Assignees:-
Labels:
area-CodeGen-coreclr
Milestone:-
@build-analysisbuild-analysisBot
mentioned this pull request
Jun 23, 2023

Closed

@Ruihan-YinRuihan-Yin
changed the title
Enabled embedded broadcast for binary opsJIT: Enabled embedded broadcast for binary opsJun 23, 2023
@Ruihan-YinRuihan-Yin
reopened this
Jun 23, 2023
@tannergoodingtannergooding
added
the 
avx512
Related to the AVX-512 architecture
label
Jun 27, 2023
@Ruihan-Yin
Ruihan-Yin
marked this pull request as ready for review
June 27, 2023 22:01
@Ruihan-Yin
Ruihan-Yin
commented
Jun 28, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Hi @tannergooding , the PR should be ready for review, would you please take a look at it?

it mostly includes:
  1. some updates on the metadata in the related tables to enable embedded broadcast in more instructions.
  2. some workaround on the bitwise instructions to make it work correctly with embedded broadcast when the input data type is in 64-bit.
  3. a bug fix where some broadcast nodes with data type less than 32 bits are accidentally contained, made some changes in the contain check to filter out these situations.
@build-analysisbuild-analysisBot
mentioned this pull request
Jun 29, 2023

Closed

tannergooding
tannergooding
reviewed
Jun 29, 2023
View reviewed changes
Comment threadsrc/coreclr/jit/instr.cpp
Outdated

@build-analysisbuild-analysisBot
mentioned this pull request
Jun 29, 2023

Closed

@Ruihan-Yin
Ruihan-Yin
commented
Jun 29, 2023

Copy link


Copy Markdown

MemberAuthor
windows x64
Diffs are based on 1,627,004 contexts (467,427 MinOpts, 1,159,577 FullOpts).
MISSED contexts: 2,227 (0.14%)
Overall (+2,406 bytes)
CollectionBase size (bytes)Diff size (bytes)
aspnet.run.windows.x64.checked.mch44,062,516+156
benchmarks.run.windows.x64.checked.mch7,244,558+152
benchmarks.run_pgo.windows.x64.checked.mch26,963,808+143
benchmarks.run_tiered.windows.x64.checked.mch10,507,436+157
coreclr_tests.run.windows.x64.checked.mch385,009,583+736
libraries.pmi.windows.x64.checked.mch59,506,418+190
libraries_tests.pmi.windows.x64.checked.mch121,311,207+220
realworld.run.windows.x64.checked.mch13,650,175+652
MinOpts (+383 bytes)
CollectionBase size (bytes)Diff size (bytes)
benchmarks.run_pgo.windows.x64.checked.mch10,473,053+73
benchmarks.run_tiered.windows.x64.checked.mch7,567,856+73
coreclr_tests.run.windows.x64.checked.mch269,424,110+237
FullOpts (+2,023 bytes)
CollectionBase size (bytes)Diff size (bytes)
aspnet.run.windows.x64.checked.mch29,278,684+156
benchmarks.run.windows.x64.checked.mch6,870,267+152
benchmarks.run_pgo.windows.x64.checked.mch16,490,755+70
benchmarks.run_tiered.windows.x64.checked.mch2,939,580+84
coreclr_tests.run.windows.x64.checked.mch115,585,473+499
libraries.pmi.windows.x64.checked.mch57,987,107+190
libraries_tests.pmi.windows.x64.checked.mch114,958,091+220
realworld.run.windows.x64.checked.mch12,500,933+652
@Ruihan-Yin
Ruihan-Yin
commented
Jun 29, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Fails should be irrelevant.
We had regression of about 2,400 bytes in the code size introduced by the changes. This should be expected as the regression is the conversion from VEX encoding to EVEX encoding when enabling embedded broadcast.

And there is no influence on the throughput.
Do we accept the results?
@tannergooding
tannergooding
commented
Jul 6, 2023

Copy link


Copy Markdown

Member
Do we accept the results?
The results look good/within reason to me. There are two important bits to call out...
First this change, in the typical case, trades a slight increase in code size (typically +2-bytes) for a decrease in data size (going from sizeof(VectorXXX<T>) to sizeof(T)) thereby increasing data locality and minimizing impact to the cache. This will often be a performance win.
Second, SPMI diffs are not currently tracking/including the data size allocations as part of "bytes of code" metric. HardwareIntrinsics.RayTracer.Packet256Tracer:Shade for example is +2 bytes of code, but -32 bytes of data so its actually a net win of -30 bytes of allocations
tannergooding
tannergooding
reviewed
Jul 6, 2023
View reviewed changes
Comment threadsrc/coreclr/jit/lowerxarch.cpp
Outdated

@tannergoodingtannergoodingJul 6, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This is better handled as

Suggested change

if (baseType == TYP_BYTE || baseType == TYP_UBYTE || baseType == TYP_SHORT || baseType == TYP_USHORT)
if (varTypeIsSmall(baseType))
@tannergooding
tannergooding
commented
Jul 6, 2023

Copy link


Copy Markdown

Member
CC. @dotnet/jit-contrib for secondary review.
@BruceForstall
BruceForstall
commented
Jul 6, 2023

Copy link


Copy Markdown

Contributor
superpmi-replay and superpmi-diffs pipelines are failing with messages like:
[17:40:21] ERROR: Couldn't load base metrics summary created by child process
[17:40:21] General fatal error

I wonder if the JIT is crashing (not asserting) with this PR?
@Ruihan-Yin
Ruihan-Yin
commented
Jul 6, 2023

Copy link


Copy Markdown

MemberAuthor
I wonder if the JIT is crashing (not asserting) with this PR?
First time seeing this fail in this PR, the recent changes might not cause the crashing, I will rebase the PR and re-run the CI again to confirm.
BruceForstall
BruceForstall
reviewed
Jul 7, 2023
View reviewed changes
Comment threadsrc/coreclr/jit/hwintrinsic.h
Outdated

@BruceForstallBruceForstallJul 6, 2023

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.

This seems like an odd location for this function. All the other functions here take a NamedIntrinsic. Should this instead be in emitxarch.h/cpp like IsMovInstruction or HasKMaskRegisterDest ?
@Ruihan-YinRuihan-YinJul 10, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

moved the function into emitxarch.h/cpp as a static function.
Comment threadsrc/coreclr/jit/instrsxarch.h
Outdated

@BruceForstallBruceForstallJul 7, 2023

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.

nit: no need to delete this empty line?
@tannergooding
tannergooding
commented
Jul 7, 2023

Copy link


Copy Markdown

Member
I wonder if the JIT is crashing (not asserting) with this PR?
There are similar failures in multiple PRs from what I've seen recently.
Noting that SPMI diffs is faulting in Python when processing the diffs summary:
Traceback (most recent call last):
File "C:\h\w\B2080989\p\superpmi.py", line 4667, in <module>
sys.exit(main(args))
File "C:\h\w\B2080989\p\superpmi.py", line 4558, in main
success = asm_diffs.replay_with_asm_diffs()
File "C:\h\w\B2080989\p\superpmi.py", line 2019, in replay_with_asm_diffs
self.write_asmdiffs_markdown_summary(write_fh, asm_diffs)
File "C:\h\w\B2080989\p\superpmi.py", line 2170, in write_asmdiffs_markdown_summary
write_row(*t)
File "C:\h\w\B2080989\p\superpmi.py", line 2165, in write_row
num_missed_base / num_contexts * 100,
ZeroDivisionError: division by zero

Replay is similarly failing with things like:
[17:42:50] Invoking: C:\h\w\B6DA095E\p\superpmi.exe -v ewi -r C:\h\w\B6DA095E\t\tmpdsiibugo\repro -p -jitoption JitStressRegs=0x80 -f C:\h\w\B6DA095E\t\tmpdsiibugo\coreclr_tests.run.windows.x64.checked.mch_fail.mcl -metricsSummary C:\h\w\B6DA095E\t\tmpdsiibugo\coreclr_tests.run.windows.x64.checked.mch_metrics.csv C:\h\w\B6DA095E\p\clrjit_win_x64_x64.dll C:\h\w\B6DA095E\p\artifacts\spmi\mch\02e334af-4e6e-4a68-9feb-308d3d2661bc.windows.x64\coreclr_tests.run.windows.x64.checked.mch
[17:42:50] ERROR: Couldn't load base metrics summary created by child process
[17:42:50] ERROR: Couldn't load base metrics summary created by child process
[17:42:50] ERROR: Couldn't load base metrics summary created by child process
[17:42:50] ERROR: Couldn't load base metrics summary created by child process
[17:42:50] General fatal error
[17:42:50] Running SuperPMI replay of C:\h\w\B6DA095E\p\artifacts\spmi\mch\02e334af-4e6e-4a68-9feb-308d3d2661bc.windows.x64\libraries.crossgen2.windows.x64.checked.mch

Did we recently get a JIT/EE version change and potentially not have up to date baselines?
@BruceForstall
BruceForstall
commented
Jul 7, 2023

Copy link


Copy Markdown

Contributor
Noting that SPMI diffs is faulting in Python when processing the diffs summary:
superpmi.py should gracefully handle that. But I think that's a symptom that superpmi.exe crashed.
Did we recently get a JIT/EE version change and potentially not have up to date baselines?
If the GUID changed, we wouldn't have this problem because we would only pick up matched MCH files.
If someone changed the interface without changing the GUID, then anything is possible.
@BruceForstall
BruceForstall
commented
Jul 7, 2023

Copy link


Copy Markdown

Contributor
I kicked off runs on main (not on a PR) so we can see if there are failures in the main branch:
https://dev.azure.com/dnceng-public/public/_build/results?buildId=331428&view=results
https://dev.azure.com/dnceng-public/public/_build/results?buildId=331429&view=results
@BruceForstall
BruceForstall
commented
Jul 7, 2023

Copy link


Copy Markdown

Contributor
@Ruihan-Yin The superpmi pipelines are now running clean. I would suggest to merge up to HEAD and re-push your PR to trigger re-run of these pipelines.
@Ruihan-Yin
Ruihan-Yin
commented
Jul 7, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
The superpmi pipelines are now running clean. I would suggest to merge up to HEAD and re-push your PR to trigger re-run of these pipelines.
Thanks for the notification, will resolve the reviews and rebase the branch soon.
@Ruihan-Yin
Enabled embedded broadcast for the following ops:
5ed01d7
and, andn, or, xor,
min, max,
div, mul, mull, sub,
variable shiftleftlogical/rightarithmetic/rightlogical
@Ruihan-Yin
 Bug fix:
0f746c4
 JIT used to use a uniform intrinsic for bitwise operations with all data
types, embedded broadcast is sensitive to input size in this case,
adding a helper to let emitter aware when input size is long/ulong.
@Ruihan-Yin
reset the instruction in the later phase
7825a18
when embedded broadcast is actually enabled
@Ruihan-Yin
filter irrelevant data type in embedded broadcast
1513e55
There are cases when broadcast node are falsely contained by a embedded
broadcast compatible node, while the data type is actually not supported
Adding extra logics to avoid this situation.
@Ruihan-Yin
update the condition on instruction reset:
a326e58
instructions with either long or ulong as basetype should be reset to
qword instructions.
@Ruihan-Yin
Apply format patch
4468b9f
@Ruihan-Yin
Resolve reviews:
89120d2
make the typecheck based on broadcast node it self.
@Ruihan-Yin
Resolve review:
f31da65
use `varTypeIsSmall` type check to cover all the unsupported data type
in embedded broadcast.
@Ruihan-Yin
Resolve reviews:
23c3f6e
1. put the IsBitwiseInstruction to a proper place.
2. nit: restored unnecessary line delete.
@Ruihan-Yin
Ruihan-Yin
commented
Jul 10, 2023

Copy link


Copy Markdown

MemberAuthor
Hi @BruceForstall, some tests are cancelled for some reasons, I suppose it is not relevant to the changes in this PR, shall I re-run the test again? And can you please review the new changes, if any. Thanks!
EgorBo
EgorBo
reviewed
Jul 14, 2023
View reviewed changes
Comment threadsrc/coreclr/jit/instr.cpp
 if (IsEmbBroadcast)
 {
 instOptions = INS_OPTS_EVEX_b;
 if (emitter::IsBitwiseInstruction(ins) && varTypeIsLong(op2->AsHWIntrinsic()->GetSimdBaseType()))
@EgorBoEgorBoJul 14, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

nit: do we need IsBitwiseInstruction at all since we have a switch over ins anyway? (we can just remove the unreached in debug
@tannergoodingtannergoodingJul 14, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Possibly not. I'll let Ruihan-Yin fix this in a follow up PR if they want to, that way we can land the important bit before the snap.
@Ruihan-YinRuihan-YinJul 14, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Thanks for the feedback, we will fix this point in the following PR on embedded broadcast.
@tannergooding
tannergooding
merged commit ef9a07c
into
dotnet:mainJul 14, 2023
@JulieLeeMSFTJulieLeeMSFT
mentioned this pull request
Aug 2, 2023

Closed

3 tasks
@BruceForstallBruceForstall
mentioned this pull request
Aug 3, 2023
 Merged

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


Reviewers

@tannergoodingtannergoodingtannergooding approved these changes
@EgorBoEgorBoEgorBo approved these changes
@BruceForstallBruceForstallAwaiting requested review from BruceForstall

Assignees


No one assigned


Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMIavx512Related to the AVX-512 architecturecommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@Ruihan-Yin@tannergooding@BruceForstall@EgorBo

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Highlight search terms from Google/DuckDuckGo/Bing referrer
(function() {
var ref = document.referrer;
var terms = [];
if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {
var url = new URL(ref);
var q = url.searchParams.get('q') || url.searchParams.get('p');
if (q) {
terms = q.split(/\s+/).filter(function(t) { return t.length > 2; });
}
}
if (terms.length === 0) return;
var style = document.createElement('style');
style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';
document.head.appendChild(style);
function highlight(node) {
if (node.nodeType === 3) { // text node
var text = node.textContent;
var found = false;
terms.forEach(function(term) {
var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi');
if (regex.test(text)) {
found = true;
var frag = document.createDocumentFragment();
var parts = text.split(regex);
parts.forEach(function(part, i) {
if (i % 2 === 0) {
frag.appendChild(document.createTextNode(part));
} else {
var span = document.createElement('span');
span.className = 'userscript-highlight';
span.textContent = part;
frag.appendChild(span);
}
});
node.parentNode.replaceChild(frag, node);
}
});
} else if (node.nodeType === 1 && node.childNodes) { // element
var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];
if (!skipTags.includes(node.tagName)) {
Array.from(node.childNodes).forEach(highlight);
}
}
}
highlight(document.body);
// Re-highlight on dynamic content
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1 || node.nodeType === 3) highlight(node);
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
JIT: Enabled embedded broadcast for binary ops by Ruihan-Yin · Pull Request #87946 · dotnet/runtime · GitHub
Skip to content
JIT: Enabled embedded broadcast for binary ops - #87946
Merged
tannergooding merged 9 commits into
dotnet:mainfrom 
Ruihan-Yin:EbBinary
Jul 14, 2023
Merged
JIT: Enabled embedded broadcast for binary ops#87946
tannergooding merged 9 commits into
dotnet:mainfrom 
Ruihan-Yin:EbBinary
Conversation
@Ruihan-Yin
@Ruihan-YinRuihan-Yin
commented
Jun 22, 2023

Copy link


Copy Markdown

Member
This PR provides the embedded broadcast support for binary ops, to be more specific, ops using the genHWIntrinsic_R_R_RM path.
Including the following ops:

and, andn, or, xor,

min, max,

div, mul, mull, sub,

variable shiftleftlogical/rightarithmetic/rightlogical
@ghostghost
added

area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
community-contribution
Indicates that the PR has been added by a community member
labels
Jun 22, 2023
@ghost
ghost
commented
Jun 22, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch

See info in area-owners.md if you want to be subscribed.
Issue Details
This PR provides the embedded broadcast support for binary ops, to be more specific, ops using the genHWIntrinsic_R_R_RM path.
Including the following ops:

and, andn, or, xor,

min, max,

div, mul, mull, sub,

variable shiftleftlogical/rightarithmetic/rightlogical
Author:Ruihan-Yin
Assignees:-
Labels:
area-CodeGen-coreclr
Milestone:-
@build-analysisbuild-analysisBot
mentioned this pull request
Jun 23, 2023

Closed

@Ruihan-YinRuihan-Yin
changed the title
Enabled embedded broadcast for binary opsJIT: Enabled embedded broadcast for binary opsJun 23, 2023
@Ruihan-YinRuihan-Yin
reopened this
Jun 23, 2023
@tannergoodingtannergooding
added
the 
avx512
Related to the AVX-512 architecture
label
Jun 27, 2023
@Ruihan-Yin
Ruihan-Yin
marked this pull request as ready for review
June 27, 2023 22:01
@Ruihan-Yin
Ruihan-Yin
commented
Jun 28, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Hi @tannergooding , the PR should be ready for review, would you please take a look at it?

it mostly includes:
  1. some updates on the metadata in the related tables to enable embedded broadcast in more instructions.
  2. some workaround on the bitwise instructions to make it work correctly with embedded broadcast when the input data type is in 64-bit.
  3. a bug fix where some broadcast nodes with data type less than 32 bits are accidentally contained, made some changes in the contain check to filter out these situations.
@build-analysisbuild-analysisBot
mentioned this pull request
Jun 29, 2023

Closed

tannergooding
tannergooding
reviewed
Jun 29, 2023
View reviewed changes
Comment threadsrc/coreclr/jit/instr.cpp
Outdated

@build-analysisbuild-analysisBot
mentioned this pull request
Jun 29, 2023

Closed

@Ruihan-Yin
Ruihan-Yin
commented
Jun 29, 2023

Copy link


Copy Markdown

MemberAuthor
windows x64
Diffs are based on 1,627,004 contexts (467,427 MinOpts, 1,159,577 FullOpts).
MISSED contexts: 2,227 (0.14%)
Overall (+2,406 bytes)
CollectionBase size (bytes)Diff size (bytes)
aspnet.run.windows.x64.checked.mch44,062,516+156
benchmarks.run.windows.x64.checked.mch7,244,558+152
benchmarks.run_pgo.windows.x64.checked.mch26,963,808+143
benchmarks.run_tiered.windows.x64.checked.mch10,507,436+157
coreclr_tests.run.windows.x64.checked.mch385,009,583+736
libraries.pmi.windows.x64.checked.mch59,506,418+190
libraries_tests.pmi.windows.x64.checked.mch121,311,207+220
realworld.run.windows.x64.checked.mch13,650,175+652
MinOpts (+383 bytes)
CollectionBase size (bytes)Diff size (bytes)
benchmarks.run_pgo.windows.x64.checked.mch10,473,053+73
benchmarks.run_tiered.windows.x64.checked.mch7,567,856+73
coreclr_tests.run.windows.x64.checked.mch269,424,110+237
FullOpts (+2,023 bytes)
CollectionBase size (bytes)Diff size (bytes)
aspnet.run.windows.x64.checked.mch29,278,684+156
benchmarks.run.windows.x64.checked.mch6,870,267+152
benchmarks.run_pgo.windows.x64.checked.mch16,490,755+70
benchmarks.run_tiered.windows.x64.checked.mch2,939,580+84
coreclr_tests.run.windows.x64.checked.mch115,585,473+499
libraries.pmi.windows.x64.checked.mch57,987,107+190
libraries_tests.pmi.windows.x64.checked.mch114,958,091+220
realworld.run.windows.x64.checked.mch12,500,933+652
@Ruihan-Yin
Ruihan-Yin
commented
Jun 29, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Fails should be irrelevant.
We had regression of about 2,400 bytes in the code size introduced by the changes. This should be expected as the regression is the conversion from VEX encoding to EVEX encoding when enabling embedded broadcast.

And there is no influence on the throughput.
Do we accept the results?
@tannergooding
tannergooding
commented
Jul 6, 2023

Copy link


Copy Markdown

Member
Do we accept the results?
The results look good/within reason to me. There are two important bits to call out...
First this change, in the typical case, trades a slight increase in code size (typically +2-bytes) for a decrease in data size (going from sizeof(VectorXXX<T>) to sizeof(T)) thereby increasing data locality and minimizing impact to the cache. This will often be a performance win.
Second, SPMI diffs are not currently tracking/including the data size allocations as part of "bytes of code" metric. HardwareIntrinsics.RayTracer.Packet256Tracer:Shade for example is +2 bytes of code, but -32 bytes of data so its actually a net win of -30 bytes of allocations
tannergooding
tannergooding
reviewed
Jul 6, 2023
View reviewed changes
Comment threadsrc/coreclr/jit/lowerxarch.cpp
Outdated

@tannergoodingtannergoodingJul 6, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This is better handled as

Suggested change

if (baseType == TYP_BYTE || baseType == TYP_UBYTE || baseType == TYP_SHORT || baseType == TYP_USHORT)
if (varTypeIsSmall(baseType))
@tannergooding
tannergooding
commented
Jul 6, 2023

Copy link


Copy Markdown

Member
CC. @dotnet/jit-contrib for secondary review.
@BruceForstall
BruceForstall
commented
Jul 6, 2023

Copy link


Copy Markdown

Contributor
superpmi-replay and superpmi-diffs pipelines are failing with messages like:
[17:40:21] ERROR: Couldn't load base metrics summary created by child process
[17:40:21] General fatal error

I wonder if the JIT is crashing (not asserting) with this PR?
@Ruihan-Yin
Ruihan-Yin
commented
Jul 6, 2023

Copy link


Copy Markdown

MemberAuthor
I wonder if the JIT is crashing (not asserting) with this PR?
First time seeing this fail in this PR, the recent changes might not cause the crashing, I will rebase the PR and re-run the CI again to confirm.
BruceForstall
BruceForstall
reviewed
Jul 7, 2023
View reviewed changes
Comment threadsrc/coreclr/jit/hwintrinsic.h
Outdated

@BruceForstallBruceForstallJul 6, 2023

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.

This seems like an odd location for this function. All the other functions here take a NamedIntrinsic. Should this instead be in emitxarch.h/cpp like IsMovInstruction or HasKMaskRegisterDest ?
@Ruihan-YinRuihan-YinJul 10, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

moved the function into emitxarch.h/cpp as a static function.
Comment threadsrc/coreclr/jit/instrsxarch.h
Outdated

@BruceForstallBruceForstallJul 7, 2023

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.

nit: no need to delete this empty line?
@tannergooding
tannergooding
commented
Jul 7, 2023

Copy link


Copy Markdown

Member
I wonder if the JIT is crashing (not asserting) with this PR?
There are similar failures in multiple PRs from what I've seen recently.
Noting that SPMI diffs is faulting in Python when processing the diffs summary:
Traceback (most recent call last):
File "C:\h\w\B2080989\p\superpmi.py", line 4667, in <module>
sys.exit(main(args))
File "C:\h\w\B2080989\p\superpmi.py", line 4558, in main
success = asm_diffs.replay_with_asm_diffs()
File "C:\h\w\B2080989\p\superpmi.py", line 2019, in replay_with_asm_diffs
self.write_asmdiffs_markdown_summary(write_fh, asm_diffs)
File "C:\h\w\B2080989\p\superpmi.py", line 2170, in write_asmdiffs_markdown_summary
write_row(*t)
File "C:\h\w\B2080989\p\superpmi.py", line 2165, in write_row
num_missed_base / num_contexts * 100,
ZeroDivisionError: division by zero

Replay is similarly failing with things like:
[17:42:50] Invoking: C:\h\w\B6DA095E\p\superpmi.exe -v ewi -r C:\h\w\B6DA095E\t\tmpdsiibugo\repro -p -jitoption JitStressRegs=0x80 -f C:\h\w\B6DA095E\t\tmpdsiibugo\coreclr_tests.run.windows.x64.checked.mch_fail.mcl -metricsSummary C:\h\w\B6DA095E\t\tmpdsiibugo\coreclr_tests.run.windows.x64.checked.mch_metrics.csv C:\h\w\B6DA095E\p\clrjit_win_x64_x64.dll C:\h\w\B6DA095E\p\artifacts\spmi\mch\02e334af-4e6e-4a68-9feb-308d3d2661bc.windows.x64\coreclr_tests.run.windows.x64.checked.mch
[17:42:50] ERROR: Couldn't load base metrics summary created by child process
[17:42:50] ERROR: Couldn't load base metrics summary created by child process
[17:42:50] ERROR: Couldn't load base metrics summary created by child process
[17:42:50] ERROR: Couldn't load base metrics summary created by child process
[17:42:50] General fatal error
[17:42:50] Running SuperPMI replay of C:\h\w\B6DA095E\p\artifacts\spmi\mch\02e334af-4e6e-4a68-9feb-308d3d2661bc.windows.x64\libraries.crossgen2.windows.x64.checked.mch

Did we recently get a JIT/EE version change and potentially not have up to date baselines?
@BruceForstall
BruceForstall
commented
Jul 7, 2023

Copy link


Copy Markdown

Contributor
Noting that SPMI diffs is faulting in Python when processing the diffs summary:
superpmi.py should gracefully handle that. But I think that's a symptom that superpmi.exe crashed.
Did we recently get a JIT/EE version change and potentially not have up to date baselines?
If the GUID changed, we wouldn't have this problem because we would only pick up matched MCH files.
If someone changed the interface without changing the GUID, then anything is possible.
@BruceForstall
BruceForstall
commented
Jul 7, 2023

Copy link


Copy Markdown

Contributor
I kicked off runs on main (not on a PR) so we can see if there are failures in the main branch:
https://dev.azure.com/dnceng-public/public/_build/results?buildId=331428&view=results
https://dev.azure.com/dnceng-public/public/_build/results?buildId=331429&view=results
@BruceForstall
BruceForstall
commented
Jul 7, 2023

Copy link


Copy Markdown

Contributor
@Ruihan-Yin The superpmi pipelines are now running clean. I would suggest to merge up to HEAD and re-push your PR to trigger re-run of these pipelines.
@Ruihan-Yin
Ruihan-Yin
commented
Jul 7, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
The superpmi pipelines are now running clean. I would suggest to merge up to HEAD and re-push your PR to trigger re-run of these pipelines.
Thanks for the notification, will resolve the reviews and rebase the branch soon.
@Ruihan-Yin
Enabled embedded broadcast for the following ops:
5ed01d7
and, andn, or, xor,
min, max,
div, mul, mull, sub,
variable shiftleftlogical/rightarithmetic/rightlogical
@Ruihan-Yin
 Bug fix:
0f746c4
 JIT used to use a uniform intrinsic for bitwise operations with all data
types, embedded broadcast is sensitive to input size in this case,
adding a helper to let emitter aware when input size is long/ulong.
@Ruihan-Yin
reset the instruction in the later phase
7825a18
when embedded broadcast is actually enabled
@Ruihan-Yin
filter irrelevant data type in embedded broadcast
1513e55
There are cases when broadcast node are falsely contained by a embedded
broadcast compatible node, while the data type is actually not supported
Adding extra logics to avoid this situation.
@Ruihan-Yin
update the condition on instruction reset:
a326e58
instructions with either long or ulong as basetype should be reset to
qword instructions.
@Ruihan-Yin
Apply format patch
4468b9f
@Ruihan-Yin
Resolve reviews:
89120d2
make the typecheck based on broadcast node it self.
@Ruihan-Yin
Resolve review:
f31da65
use `varTypeIsSmall` type check to cover all the unsupported data type
in embedded broadcast.
@Ruihan-Yin
Resolve reviews:
23c3f6e
1. put the IsBitwiseInstruction to a proper place.
2. nit: restored unnecessary line delete.
@Ruihan-Yin
Ruihan-Yin
commented
Jul 10, 2023

Copy link


Copy Markdown

MemberAuthor
Hi @BruceForstall, some tests are cancelled for some reasons, I suppose it is not relevant to the changes in this PR, shall I re-run the test again? And can you please review the new changes, if any. Thanks!
EgorBo
EgorBo
reviewed
Jul 14, 2023
View reviewed changes
Comment threadsrc/coreclr/jit/instr.cpp
 if (IsEmbBroadcast)
 {
 instOptions = INS_OPTS_EVEX_b;
 if (emitter::IsBitwiseInstruction(ins) && varTypeIsLong(op2->AsHWIntrinsic()->GetSimdBaseType()))
@EgorBoEgorBoJul 14, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

nit: do we need IsBitwiseInstruction at all since we have a switch over ins anyway? (we can just remove the unreached in debug
@tannergoodingtannergoodingJul 14, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Possibly not. I'll let Ruihan-Yin fix this in a follow up PR if they want to, that way we can land the important bit before the snap.
@Ruihan-YinRuihan-YinJul 14, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Thanks for the feedback, we will fix this point in the following PR on embedded broadcast.
@tannergooding
tannergooding
merged commit ef9a07c
into
dotnet:mainJul 14, 2023
@JulieLeeMSFTJulieLeeMSFT
mentioned this pull request
Aug 2, 2023

Closed

3 tasks
@BruceForstallBruceForstall
mentioned this pull request
Aug 3, 2023
 Merged

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


Reviewers

@tannergoodingtannergoodingtannergooding approved these changes
@EgorBoEgorBoEgorBo approved these changes
@BruceForstallBruceForstallAwaiting requested review from BruceForstall

Assignees


No one assigned


Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMIavx512Related to the AVX-512 architecturecommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@Ruihan-Yin@tannergooding@BruceForstall@EgorBo

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Strip utm_, fbclid, gclid, etc. from all links on page
(function() {
var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',
'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',
'ref', 'ref_src', 'source', 'medium', 'campaign'];
function cleanUrl(url) {
try {
var u = new URL(url, window.location.origin);
var changed = false;
trackingParams.forEach(function(p) {
if (u.searchParams.has(p)) {
u.searchParams.delete(p);
changed = true;
}
});
return changed ? u.toString() : url;
} catch (e) {
return url;
}
}
function cleanLinks() {
document.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
cleanLinks();
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1) {
if (node.tagName === 'A') cleanLinks();
node.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '
JIT: Enabled embedded broadcast for binary ops by Ruihan-Yin · Pull Request #87946 · dotnet/runtime · GitHub
Skip to content
JIT: Enabled embedded broadcast for binary ops - #87946
Merged
tannergooding merged 9 commits into
dotnet:mainfrom 
Ruihan-Yin:EbBinary
Jul 14, 2023
Merged
JIT: Enabled embedded broadcast for binary ops#87946
tannergooding merged 9 commits into
dotnet:mainfrom 
Ruihan-Yin:EbBinary
Conversation
@Ruihan-Yin
@Ruihan-YinRuihan-Yin
commented
Jun 22, 2023

Copy link


Copy Markdown

Member
This PR provides the embedded broadcast support for binary ops, to be more specific, ops using the genHWIntrinsic_R_R_RM path.
Including the following ops:

and, andn, or, xor,

min, max,

div, mul, mull, sub,

variable shiftleftlogical/rightarithmetic/rightlogical
@ghostghost
added

area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
community-contribution
Indicates that the PR has been added by a community member
labels
Jun 22, 2023
@ghost
ghost
commented
Jun 22, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch

See info in area-owners.md if you want to be subscribed.
Issue Details
This PR provides the embedded broadcast support for binary ops, to be more specific, ops using the genHWIntrinsic_R_R_RM path.
Including the following ops:

and, andn, or, xor,

min, max,

div, mul, mull, sub,

variable shiftleftlogical/rightarithmetic/rightlogical
Author:Ruihan-Yin
Assignees:-
Labels:
area-CodeGen-coreclr
Milestone:-
@build-analysisbuild-analysisBot
mentioned this pull request
Jun 23, 2023

Closed

@Ruihan-YinRuihan-Yin
changed the title
Enabled embedded broadcast for binary opsJIT: Enabled embedded broadcast for binary opsJun 23, 2023
@Ruihan-YinRuihan-Yin
reopened this
Jun 23, 2023
@tannergoodingtannergooding
added
the 
avx512
Related to the AVX-512 architecture
label
Jun 27, 2023
@Ruihan-Yin
Ruihan-Yin
marked this pull request as ready for review
June 27, 2023 22:01
@Ruihan-Yin
Ruihan-Yin
commented
Jun 28, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Hi @tannergooding , the PR should be ready for review, would you please take a look at it?

it mostly includes:
  1. some updates on the metadata in the related tables to enable embedded broadcast in more instructions.
  2. some workaround on the bitwise instructions to make it work correctly with embedded broadcast when the input data type is in 64-bit.
  3. a bug fix where some broadcast nodes with data type less than 32 bits are accidentally contained, made some changes in the contain check to filter out these situations.
@build-analysisbuild-analysisBot
mentioned this pull request
Jun 29, 2023

Closed

tannergooding
tannergooding
reviewed
Jun 29, 2023
View reviewed changes
Comment threadsrc/coreclr/jit/instr.cpp
Outdated

@build-analysisbuild-analysisBot
mentioned this pull request
Jun 29, 2023

Closed

@Ruihan-Yin
Ruihan-Yin
commented
Jun 29, 2023

Copy link


Copy Markdown

MemberAuthor
windows x64
Diffs are based on 1,627,004 contexts (467,427 MinOpts, 1,159,577 FullOpts).
MISSED contexts: 2,227 (0.14%)
Overall (+2,406 bytes)
CollectionBase size (bytes)Diff size (bytes)
aspnet.run.windows.x64.checked.mch44,062,516+156
benchmarks.run.windows.x64.checked.mch7,244,558+152
benchmarks.run_pgo.windows.x64.checked.mch26,963,808+143
benchmarks.run_tiered.windows.x64.checked.mch10,507,436+157
coreclr_tests.run.windows.x64.checked.mch385,009,583+736
libraries.pmi.windows.x64.checked.mch59,506,418+190
libraries_tests.pmi.windows.x64.checked.mch121,311,207+220
realworld.run.windows.x64.checked.mch13,650,175+652
MinOpts (+383 bytes)
CollectionBase size (bytes)Diff size (bytes)
benchmarks.run_pgo.windows.x64.checked.mch10,473,053+73
benchmarks.run_tiered.windows.x64.checked.mch7,567,856+73
coreclr_tests.run.windows.x64.checked.mch269,424,110+237
FullOpts (+2,023 bytes)
CollectionBase size (bytes)Diff size (bytes)
aspnet.run.windows.x64.checked.mch29,278,684+156
benchmarks.run.windows.x64.checked.mch6,870,267+152
benchmarks.run_pgo.windows.x64.checked.mch16,490,755+70
benchmarks.run_tiered.windows.x64.checked.mch2,939,580+84
coreclr_tests.run.windows.x64.checked.mch115,585,473+499
libraries.pmi.windows.x64.checked.mch57,987,107+190
libraries_tests.pmi.windows.x64.checked.mch114,958,091+220
realworld.run.windows.x64.checked.mch12,500,933+652
@Ruihan-Yin
Ruihan-Yin
commented
Jun 29, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Fails should be irrelevant.
We had regression of about 2,400 bytes in the code size introduced by the changes. This should be expected as the regression is the conversion from VEX encoding to EVEX encoding when enabling embedded broadcast.

And there is no influence on the throughput.
Do we accept the results?
@tannergooding
tannergooding
commented
Jul 6, 2023

Copy link


Copy Markdown

Member
Do we accept the results?
The results look good/within reason to me. There are two important bits to call out...
First this change, in the typical case, trades a slight increase in code size (typically +2-bytes) for a decrease in data size (going from sizeof(VectorXXX<T>) to sizeof(T)) thereby increasing data locality and minimizing impact to the cache. This will often be a performance win.
Second, SPMI diffs are not currently tracking/including the data size allocations as part of "bytes of code" metric. HardwareIntrinsics.RayTracer.Packet256Tracer:Shade for example is +2 bytes of code, but -32 bytes of data so its actually a net win of -30 bytes of allocations
tannergooding
tannergooding
reviewed
Jul 6, 2023
View reviewed changes
Comment threadsrc/coreclr/jit/lowerxarch.cpp
Outdated

@tannergoodingtannergoodingJul 6, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This is better handled as

Suggested change

if (baseType == TYP_BYTE || baseType == TYP_UBYTE || baseType == TYP_SHORT || baseType == TYP_USHORT)
if (varTypeIsSmall(baseType))
@tannergooding
tannergooding
commented
Jul 6, 2023

Copy link


Copy Markdown

Member
CC. @dotnet/jit-contrib for secondary review.
@BruceForstall
BruceForstall
commented
Jul 6, 2023

Copy link


Copy Markdown

Contributor
superpmi-replay and superpmi-diffs pipelines are failing with messages like:
[17:40:21] ERROR: Couldn't load base metrics summary created by child process
[17:40:21] General fatal error

I wonder if the JIT is crashing (not asserting) with this PR?
@Ruihan-Yin
Ruihan-Yin
commented
Jul 6, 2023

Copy link


Copy Markdown

MemberAuthor
I wonder if the JIT is crashing (not asserting) with this PR?
First time seeing this fail in this PR, the recent changes might not cause the crashing, I will rebase the PR and re-run the CI again to confirm.
BruceForstall
BruceForstall
reviewed
Jul 7, 2023
View reviewed changes
Comment threadsrc/coreclr/jit/hwintrinsic.h
Outdated

@BruceForstallBruceForstallJul 6, 2023

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.

This seems like an odd location for this function. All the other functions here take a NamedIntrinsic. Should this instead be in emitxarch.h/cpp like IsMovInstruction or HasKMaskRegisterDest ?
@Ruihan-YinRuihan-YinJul 10, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

moved the function into emitxarch.h/cpp as a static function.
Comment threadsrc/coreclr/jit/instrsxarch.h
Outdated

@BruceForstallBruceForstallJul 7, 2023

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.

nit: no need to delete this empty line?
@tannergooding
tannergooding
commented
Jul 7, 2023

Copy link


Copy Markdown

Member
I wonder if the JIT is crashing (not asserting) with this PR?
There are similar failures in multiple PRs from what I've seen recently.
Noting that SPMI diffs is faulting in Python when processing the diffs summary:
Traceback (most recent call last):
File "C:\h\w\B2080989\p\superpmi.py", line 4667, in <module>
sys.exit(main(args))
File "C:\h\w\B2080989\p\superpmi.py", line 4558, in main
success = asm_diffs.replay_with_asm_diffs()
File "C:\h\w\B2080989\p\superpmi.py", line 2019, in replay_with_asm_diffs
self.write_asmdiffs_markdown_summary(write_fh, asm_diffs)
File "C:\h\w\B2080989\p\superpmi.py", line 2170, in write_asmdiffs_markdown_summary
write_row(*t)
File "C:\h\w\B2080989\p\superpmi.py", line 2165, in write_row
num_missed_base / num_contexts * 100,
ZeroDivisionError: division by zero

Replay is similarly failing with things like:
[17:42:50] Invoking: C:\h\w\B6DA095E\p\superpmi.exe -v ewi -r C:\h\w\B6DA095E\t\tmpdsiibugo\repro -p -jitoption JitStressRegs=0x80 -f C:\h\w\B6DA095E\t\tmpdsiibugo\coreclr_tests.run.windows.x64.checked.mch_fail.mcl -metricsSummary C:\h\w\B6DA095E\t\tmpdsiibugo\coreclr_tests.run.windows.x64.checked.mch_metrics.csv C:\h\w\B6DA095E\p\clrjit_win_x64_x64.dll C:\h\w\B6DA095E\p\artifacts\spmi\mch\02e334af-4e6e-4a68-9feb-308d3d2661bc.windows.x64\coreclr_tests.run.windows.x64.checked.mch
[17:42:50] ERROR: Couldn't load base metrics summary created by child process
[17:42:50] ERROR: Couldn't load base metrics summary created by child process
[17:42:50] ERROR: Couldn't load base metrics summary created by child process
[17:42:50] ERROR: Couldn't load base metrics summary created by child process
[17:42:50] General fatal error
[17:42:50] Running SuperPMI replay of C:\h\w\B6DA095E\p\artifacts\spmi\mch\02e334af-4e6e-4a68-9feb-308d3d2661bc.windows.x64\libraries.crossgen2.windows.x64.checked.mch

Did we recently get a JIT/EE version change and potentially not have up to date baselines?
@BruceForstall
BruceForstall
commented
Jul 7, 2023

Copy link


Copy Markdown

Contributor
Noting that SPMI diffs is faulting in Python when processing the diffs summary:
superpmi.py should gracefully handle that. But I think that's a symptom that superpmi.exe crashed.
Did we recently get a JIT/EE version change and potentially not have up to date baselines?
If the GUID changed, we wouldn't have this problem because we would only pick up matched MCH files.
If someone changed the interface without changing the GUID, then anything is possible.
@BruceForstall
BruceForstall
commented
Jul 7, 2023

Copy link


Copy Markdown

Contributor
I kicked off runs on main (not on a PR) so we can see if there are failures in the main branch:
https://dev.azure.com/dnceng-public/public/_build/results?buildId=331428&view=results
https://dev.azure.com/dnceng-public/public/_build/results?buildId=331429&view=results
@BruceForstall
BruceForstall
commented
Jul 7, 2023

Copy link


Copy Markdown

Contributor
@Ruihan-Yin The superpmi pipelines are now running clean. I would suggest to merge up to HEAD and re-push your PR to trigger re-run of these pipelines.
@Ruihan-Yin
Ruihan-Yin
commented
Jul 7, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
The superpmi pipelines are now running clean. I would suggest to merge up to HEAD and re-push your PR to trigger re-run of these pipelines.
Thanks for the notification, will resolve the reviews and rebase the branch soon.
@Ruihan-Yin
Enabled embedded broadcast for the following ops:
5ed01d7
and, andn, or, xor,
min, max,
div, mul, mull, sub,
variable shiftleftlogical/rightarithmetic/rightlogical
@Ruihan-Yin
 Bug fix:
0f746c4
 JIT used to use a uniform intrinsic for bitwise operations with all data
types, embedded broadcast is sensitive to input size in this case,
adding a helper to let emitter aware when input size is long/ulong.
@Ruihan-Yin
reset the instruction in the later phase
7825a18
when embedded broadcast is actually enabled
@Ruihan-Yin
filter irrelevant data type in embedded broadcast
1513e55
There are cases when broadcast node are falsely contained by a embedded
broadcast compatible node, while the data type is actually not supported
Adding extra logics to avoid this situation.
@Ruihan-Yin
update the condition on instruction reset:
a326e58
instructions with either long or ulong as basetype should be reset to
qword instructions.
@Ruihan-Yin
Apply format patch
4468b9f
@Ruihan-Yin
Resolve reviews:
89120d2
make the typecheck based on broadcast node it self.
@Ruihan-Yin
Resolve review:
f31da65
use `varTypeIsSmall` type check to cover all the unsupported data type
in embedded broadcast.
@Ruihan-Yin
Resolve reviews:
23c3f6e
1. put the IsBitwiseInstruction to a proper place.
2. nit: restored unnecessary line delete.
@Ruihan-Yin
Ruihan-Yin
commented
Jul 10, 2023

Copy link


Copy Markdown

MemberAuthor
Hi @BruceForstall, some tests are cancelled for some reasons, I suppose it is not relevant to the changes in this PR, shall I re-run the test again? And can you please review the new changes, if any. Thanks!
EgorBo
EgorBo
reviewed
Jul 14, 2023
View reviewed changes
Comment threadsrc/coreclr/jit/instr.cpp
 if (IsEmbBroadcast)
 {
 instOptions = INS_OPTS_EVEX_b;
 if (emitter::IsBitwiseInstruction(ins) && varTypeIsLong(op2->AsHWIntrinsic()->GetSimdBaseType()))
@EgorBoEgorBoJul 14, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

nit: do we need IsBitwiseInstruction at all since we have a switch over ins anyway? (we can just remove the unreached in debug
@tannergoodingtannergoodingJul 14, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Possibly not. I'll let Ruihan-Yin fix this in a follow up PR if they want to, that way we can land the important bit before the snap.
@Ruihan-YinRuihan-YinJul 14, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Thanks for the feedback, we will fix this point in the following PR on embedded broadcast.
@tannergooding
tannergooding
merged commit ef9a07c
into
dotnet:mainJul 14, 2023
@JulieLeeMSFTJulieLeeMSFT
mentioned this pull request
Aug 2, 2023

Closed

3 tasks
@BruceForstallBruceForstall
mentioned this pull request
Aug 3, 2023
 Merged

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


Reviewers

@tannergoodingtannergoodingtannergooding approved these changes
@EgorBoEgorBoEgorBo approved these changes
@BruceForstallBruceForstallAwaiting requested review from BruceForstall

Assignees


No one assigned


Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMIavx512Related to the AVX-512 architecturecommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@Ruihan-Yin@tannergooding@BruceForstall@EgorBo

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Auto-enable theater mode on YouTube
(function() {
function tryTheater() {
var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]');
if (btn && !btn.classList.contains('activated')) {
btn.click();
}
}
// Try immediately
tryTheater();
// Try after navigation (SPA)
var lastUrl = location.href;
setInterval(function() {
if (location.href !== lastUrl) {
lastUrl = location.href;
setTimeout(tryTheater, 500);
}
}, 1000);
// Also try on player load
var observer = new MutationObserver(tryTheater);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
JIT: Enabled embedded broadcast for binary ops by Ruihan-Yin · Pull Request #87946 · dotnet/runtime · GitHub
Skip to content
JIT: Enabled embedded broadcast for binary ops - #87946
Merged
tannergooding merged 9 commits into
dotnet:mainfrom 
Ruihan-Yin:EbBinary
Jul 14, 2023
Merged
JIT: Enabled embedded broadcast for binary ops#87946
tannergooding merged 9 commits into
dotnet:mainfrom 
Ruihan-Yin:EbBinary
Conversation
@Ruihan-Yin
@Ruihan-YinRuihan-Yin
commented
Jun 22, 2023

Copy link


Copy Markdown

Member
This PR provides the embedded broadcast support for binary ops, to be more specific, ops using the genHWIntrinsic_R_R_RM path.
Including the following ops:

and, andn, or, xor,

min, max,

div, mul, mull, sub,

variable shiftleftlogical/rightarithmetic/rightlogical
@ghostghost
added

area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
community-contribution
Indicates that the PR has been added by a community member
labels
Jun 22, 2023
@ghost
ghost
commented
Jun 22, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch

See info in area-owners.md if you want to be subscribed.
Issue Details
This PR provides the embedded broadcast support for binary ops, to be more specific, ops using the genHWIntrinsic_R_R_RM path.
Including the following ops:

and, andn, or, xor,

min, max,

div, mul, mull, sub,

variable shiftleftlogical/rightarithmetic/rightlogical
Author:Ruihan-Yin
Assignees:-
Labels:
area-CodeGen-coreclr
Milestone:-
@build-analysisbuild-analysisBot
mentioned this pull request
Jun 23, 2023

Closed

@Ruihan-YinRuihan-Yin
changed the title
Enabled embedded broadcast for binary opsJIT: Enabled embedded broadcast for binary opsJun 23, 2023
@Ruihan-YinRuihan-Yin
reopened this
Jun 23, 2023
@tannergoodingtannergooding
added
the 
avx512
Related to the AVX-512 architecture
label
Jun 27, 2023
@Ruihan-Yin
Ruihan-Yin
marked this pull request as ready for review
June 27, 2023 22:01
@Ruihan-Yin
Ruihan-Yin
commented
Jun 28, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Hi @tannergooding , the PR should be ready for review, would you please take a look at it?

it mostly includes:
  1. some updates on the metadata in the related tables to enable embedded broadcast in more instructions.
  2. some workaround on the bitwise instructions to make it work correctly with embedded broadcast when the input data type is in 64-bit.
  3. a bug fix where some broadcast nodes with data type less than 32 bits are accidentally contained, made some changes in the contain check to filter out these situations.
@build-analysisbuild-analysisBot
mentioned this pull request
Jun 29, 2023

Closed

tannergooding
tannergooding
reviewed
Jun 29, 2023
View reviewed changes
Comment threadsrc/coreclr/jit/instr.cpp
Outdated

@build-analysisbuild-analysisBot
mentioned this pull request
Jun 29, 2023

Closed

@Ruihan-Yin
Ruihan-Yin
commented
Jun 29, 2023

Copy link


Copy Markdown

MemberAuthor
windows x64
Diffs are based on 1,627,004 contexts (467,427 MinOpts, 1,159,577 FullOpts).
MISSED contexts: 2,227 (0.14%)
Overall (+2,406 bytes)
CollectionBase size (bytes)Diff size (bytes)
aspnet.run.windows.x64.checked.mch44,062,516+156
benchmarks.run.windows.x64.checked.mch7,244,558+152
benchmarks.run_pgo.windows.x64.checked.mch26,963,808+143
benchmarks.run_tiered.windows.x64.checked.mch10,507,436+157
coreclr_tests.run.windows.x64.checked.mch385,009,583+736
libraries.pmi.windows.x64.checked.mch59,506,418+190
libraries_tests.pmi.windows.x64.checked.mch121,311,207+220
realworld.run.windows.x64.checked.mch13,650,175+652
MinOpts (+383 bytes)
CollectionBase size (bytes)Diff size (bytes)
benchmarks.run_pgo.windows.x64.checked.mch10,473,053+73
benchmarks.run_tiered.windows.x64.checked.mch7,567,856+73
coreclr_tests.run.windows.x64.checked.mch269,424,110+237
FullOpts (+2,023 bytes)
CollectionBase size (bytes)Diff size (bytes)
aspnet.run.windows.x64.checked.mch29,278,684+156
benchmarks.run.windows.x64.checked.mch6,870,267+152
benchmarks.run_pgo.windows.x64.checked.mch16,490,755+70
benchmarks.run_tiered.windows.x64.checked.mch2,939,580+84
coreclr_tests.run.windows.x64.checked.mch115,585,473+499
libraries.pmi.windows.x64.checked.mch57,987,107+190
libraries_tests.pmi.windows.x64.checked.mch114,958,091+220
realworld.run.windows.x64.checked.mch12,500,933+652
@Ruihan-Yin
Ruihan-Yin
commented
Jun 29, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Fails should be irrelevant.
We had regression of about 2,400 bytes in the code size introduced by the changes. This should be expected as the regression is the conversion from VEX encoding to EVEX encoding when enabling embedded broadcast.

And there is no influence on the throughput.
Do we accept the results?
@tannergooding
tannergooding
commented
Jul 6, 2023

Copy link


Copy Markdown

Member
Do we accept the results?
The results look good/within reason to me. There are two important bits to call out...
First this change, in the typical case, trades a slight increase in code size (typically +2-bytes) for a decrease in data size (going from sizeof(VectorXXX<T>) to sizeof(T)) thereby increasing data locality and minimizing impact to the cache. This will often be a performance win.
Second, SPMI diffs are not currently tracking/including the data size allocations as part of "bytes of code" metric. HardwareIntrinsics.RayTracer.Packet256Tracer:Shade for example is +2 bytes of code, but -32 bytes of data so its actually a net win of -30 bytes of allocations
tannergooding
tannergooding
reviewed
Jul 6, 2023
View reviewed changes
Comment threadsrc/coreclr/jit/lowerxarch.cpp
Outdated

@tannergoodingtannergoodingJul 6, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This is better handled as

Suggested change

if (baseType == TYP_BYTE || baseType == TYP_UBYTE || baseType == TYP_SHORT || baseType == TYP_USHORT)
if (varTypeIsSmall(baseType))
@tannergooding
tannergooding
commented
Jul 6, 2023

Copy link


Copy Markdown

Member
CC. @dotnet/jit-contrib for secondary review.
@BruceForstall
BruceForstall
commented
Jul 6, 2023

Copy link


Copy Markdown

Contributor
superpmi-replay and superpmi-diffs pipelines are failing with messages like:
[17:40:21] ERROR: Couldn't load base metrics summary created by child process
[17:40:21] General fatal error

I wonder if the JIT is crashing (not asserting) with this PR?
@Ruihan-Yin
Ruihan-Yin
commented
Jul 6, 2023

Copy link


Copy Markdown

MemberAuthor
I wonder if the JIT is crashing (not asserting) with this PR?
First time seeing this fail in this PR, the recent changes might not cause the crashing, I will rebase the PR and re-run the CI again to confirm.
BruceForstall
BruceForstall
reviewed
Jul 7, 2023
View reviewed changes
Comment threadsrc/coreclr/jit/hwintrinsic.h
Outdated

@BruceForstallBruceForstallJul 6, 2023

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.

This seems like an odd location for this function. All the other functions here take a NamedIntrinsic. Should this instead be in emitxarch.h/cpp like IsMovInstruction or HasKMaskRegisterDest ?
@Ruihan-YinRuihan-YinJul 10, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

moved the function into emitxarch.h/cpp as a static function.
Comment threadsrc/coreclr/jit/instrsxarch.h
Outdated

@BruceForstallBruceForstallJul 7, 2023

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.

nit: no need to delete this empty line?
@tannergooding
tannergooding
commented
Jul 7, 2023

Copy link


Copy Markdown

Member
I wonder if the JIT is crashing (not asserting) with this PR?
There are similar failures in multiple PRs from what I've seen recently.
Noting that SPMI diffs is faulting in Python when processing the diffs summary:
Traceback (most recent call last):
File "C:\h\w\B2080989\p\superpmi.py", line 4667, in <module>
sys.exit(main(args))
File "C:\h\w\B2080989\p\superpmi.py", line 4558, in main
success = asm_diffs.replay_with_asm_diffs()
File "C:\h\w\B2080989\p\superpmi.py", line 2019, in replay_with_asm_diffs
self.write_asmdiffs_markdown_summary(write_fh, asm_diffs)
File "C:\h\w\B2080989\p\superpmi.py", line 2170, in write_asmdiffs_markdown_summary
write_row(*t)
File "C:\h\w\B2080989\p\superpmi.py", line 2165, in write_row
num_missed_base / num_contexts * 100,
ZeroDivisionError: division by zero

Replay is similarly failing with things like:
[17:42:50] Invoking: C:\h\w\B6DA095E\p\superpmi.exe -v ewi -r C:\h\w\B6DA095E\t\tmpdsiibugo\repro -p -jitoption JitStressRegs=0x80 -f C:\h\w\B6DA095E\t\tmpdsiibugo\coreclr_tests.run.windows.x64.checked.mch_fail.mcl -metricsSummary C:\h\w\B6DA095E\t\tmpdsiibugo\coreclr_tests.run.windows.x64.checked.mch_metrics.csv C:\h\w\B6DA095E\p\clrjit_win_x64_x64.dll C:\h\w\B6DA095E\p\artifacts\spmi\mch\02e334af-4e6e-4a68-9feb-308d3d2661bc.windows.x64\coreclr_tests.run.windows.x64.checked.mch
[17:42:50] ERROR: Couldn't load base metrics summary created by child process
[17:42:50] ERROR: Couldn't load base metrics summary created by child process
[17:42:50] ERROR: Couldn't load base metrics summary created by child process
[17:42:50] ERROR: Couldn't load base metrics summary created by child process
[17:42:50] General fatal error
[17:42:50] Running SuperPMI replay of C:\h\w\B6DA095E\p\artifacts\spmi\mch\02e334af-4e6e-4a68-9feb-308d3d2661bc.windows.x64\libraries.crossgen2.windows.x64.checked.mch

Did we recently get a JIT/EE version change and potentially not have up to date baselines?
@BruceForstall
BruceForstall
commented
Jul 7, 2023

Copy link


Copy Markdown

Contributor
Noting that SPMI diffs is faulting in Python when processing the diffs summary:
superpmi.py should gracefully handle that. But I think that's a symptom that superpmi.exe crashed.
Did we recently get a JIT/EE version change and potentially not have up to date baselines?
If the GUID changed, we wouldn't have this problem because we would only pick up matched MCH files.
If someone changed the interface without changing the GUID, then anything is possible.
@BruceForstall
BruceForstall
commented
Jul 7, 2023

Copy link


Copy Markdown

Contributor
I kicked off runs on main (not on a PR) so we can see if there are failures in the main branch:
https://dev.azure.com/dnceng-public/public/_build/results?buildId=331428&view=results
https://dev.azure.com/dnceng-public/public/_build/results?buildId=331429&view=results
@BruceForstall
BruceForstall
commented
Jul 7, 2023

Copy link


Copy Markdown

Contributor
@Ruihan-Yin The superpmi pipelines are now running clean. I would suggest to merge up to HEAD and re-push your PR to trigger re-run of these pipelines.
@Ruihan-Yin
Ruihan-Yin
commented
Jul 7, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
The superpmi pipelines are now running clean. I would suggest to merge up to HEAD and re-push your PR to trigger re-run of these pipelines.
Thanks for the notification, will resolve the reviews and rebase the branch soon.
@Ruihan-Yin
Enabled embedded broadcast for the following ops:
5ed01d7
and, andn, or, xor,
min, max,
div, mul, mull, sub,
variable shiftleftlogical/rightarithmetic/rightlogical
@Ruihan-Yin
 Bug fix:
0f746c4
 JIT used to use a uniform intrinsic for bitwise operations with all data
types, embedded broadcast is sensitive to input size in this case,
adding a helper to let emitter aware when input size is long/ulong.
@Ruihan-Yin
reset the instruction in the later phase
7825a18
when embedded broadcast is actually enabled
@Ruihan-Yin
filter irrelevant data type in embedded broadcast
1513e55
There are cases when broadcast node are falsely contained by a embedded
broadcast compatible node, while the data type is actually not supported
Adding extra logics to avoid this situation.
@Ruihan-Yin
update the condition on instruction reset:
a326e58
instructions with either long or ulong as basetype should be reset to
qword instructions.
@Ruihan-Yin
Apply format patch
4468b9f
@Ruihan-Yin
Resolve reviews:
89120d2
make the typecheck based on broadcast node it self.
@Ruihan-Yin
Resolve review:
f31da65
use `varTypeIsSmall` type check to cover all the unsupported data type
in embedded broadcast.
@Ruihan-Yin
Resolve reviews:
23c3f6e
1. put the IsBitwiseInstruction to a proper place.
2. nit: restored unnecessary line delete.
@Ruihan-Yin
Ruihan-Yin
commented
Jul 10, 2023

Copy link


Copy Markdown

MemberAuthor
Hi @BruceForstall, some tests are cancelled for some reasons, I suppose it is not relevant to the changes in this PR, shall I re-run the test again? And can you please review the new changes, if any. Thanks!
EgorBo
EgorBo
reviewed
Jul 14, 2023
View reviewed changes
Comment threadsrc/coreclr/jit/instr.cpp
 if (IsEmbBroadcast)
 {
 instOptions = INS_OPTS_EVEX_b;
 if (emitter::IsBitwiseInstruction(ins) && varTypeIsLong(op2->AsHWIntrinsic()->GetSimdBaseType()))
@EgorBoEgorBoJul 14, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

nit: do we need IsBitwiseInstruction at all since we have a switch over ins anyway? (we can just remove the unreached in debug
@tannergoodingtannergoodingJul 14, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Possibly not. I'll let Ruihan-Yin fix this in a follow up PR if they want to, that way we can land the important bit before the snap.
@Ruihan-YinRuihan-YinJul 14, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Thanks for the feedback, we will fix this point in the following PR on embedded broadcast.
@tannergooding
tannergooding
merged commit ef9a07c
into
dotnet:mainJul 14, 2023
@JulieLeeMSFTJulieLeeMSFT
mentioned this pull request
Aug 2, 2023

Closed

3 tasks
@BruceForstallBruceForstall
mentioned this pull request
Aug 3, 2023
 Merged

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


Reviewers

@tannergoodingtannergoodingtannergooding approved these changes
@EgorBoEgorBoEgorBo approved these changes
@BruceForstallBruceForstallAwaiting requested review from BruceForstall

Assignees


No one assigned


Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMIavx512Related to the AVX-512 architecturecommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@Ruihan-Yin@tannergooding@BruceForstall@EgorBo

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Remove or un-stick sticky/fixed headers that block content
(function() {
function unstick() {
document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) {
if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {
el.style.position = 'static';
el.style.top = 'auto';
el.style.zIndex = 'auto';
}
});
}
unstick();
var observer = new MutationObserver(unstick);
observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });
})();
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
JIT: Enabled embedded broadcast for binary ops by Ruihan-Yin · Pull Request #87946 · dotnet/runtime · GitHub
Skip to content
JIT: Enabled embedded broadcast for binary ops - #87946
Merged
tannergooding merged 9 commits into
dotnet:mainfrom 
Ruihan-Yin:EbBinary
Jul 14, 2023
Merged
JIT: Enabled embedded broadcast for binary ops#87946
tannergooding merged 9 commits into
dotnet:mainfrom 
Ruihan-Yin:EbBinary
Conversation
@Ruihan-Yin
@Ruihan-YinRuihan-Yin
commented
Jun 22, 2023

Copy link


Copy Markdown

Member
This PR provides the embedded broadcast support for binary ops, to be more specific, ops using the genHWIntrinsic_R_R_RM path.
Including the following ops:

and, andn, or, xor,

min, max,

div, mul, mull, sub,

variable shiftleftlogical/rightarithmetic/rightlogical
@ghostghost
added

area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
community-contribution
Indicates that the PR has been added by a community member
labels
Jun 22, 2023
@ghost
ghost
commented
Jun 22, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch

See info in area-owners.md if you want to be subscribed.
Issue Details
This PR provides the embedded broadcast support for binary ops, to be more specific, ops using the genHWIntrinsic_R_R_RM path.
Including the following ops:

and, andn, or, xor,

min, max,

div, mul, mull, sub,

variable shiftleftlogical/rightarithmetic/rightlogical
Author:Ruihan-Yin
Assignees:-
Labels:
area-CodeGen-coreclr
Milestone:-
@build-analysisbuild-analysisBot
mentioned this pull request
Jun 23, 2023

Closed

@Ruihan-YinRuihan-Yin
changed the title
Enabled embedded broadcast for binary opsJIT: Enabled embedded broadcast for binary opsJun 23, 2023
@Ruihan-YinRuihan-Yin
reopened this
Jun 23, 2023
@tannergoodingtannergooding
added
the 
avx512
Related to the AVX-512 architecture
label
Jun 27, 2023
@Ruihan-Yin
Ruihan-Yin
marked this pull request as ready for review
June 27, 2023 22:01
@Ruihan-Yin
Ruihan-Yin
commented
Jun 28, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Hi @tannergooding , the PR should be ready for review, would you please take a look at it?

it mostly includes:
  1. some updates on the metadata in the related tables to enable embedded broadcast in more instructions.
  2. some workaround on the bitwise instructions to make it work correctly with embedded broadcast when the input data type is in 64-bit.
  3. a bug fix where some broadcast nodes with data type less than 32 bits are accidentally contained, made some changes in the contain check to filter out these situations.
@build-analysisbuild-analysisBot
mentioned this pull request
Jun 29, 2023

Closed

tannergooding
tannergooding
reviewed
Jun 29, 2023
View reviewed changes
Comment threadsrc/coreclr/jit/instr.cpp
Outdated

@build-analysisbuild-analysisBot
mentioned this pull request
Jun 29, 2023

Closed

@Ruihan-Yin
Ruihan-Yin
commented
Jun 29, 2023

Copy link


Copy Markdown

MemberAuthor
windows x64
Diffs are based on 1,627,004 contexts (467,427 MinOpts, 1,159,577 FullOpts).
MISSED contexts: 2,227 (0.14%)
Overall (+2,406 bytes)
CollectionBase size (bytes)Diff size (bytes)
aspnet.run.windows.x64.checked.mch44,062,516+156
benchmarks.run.windows.x64.checked.mch7,244,558+152
benchmarks.run_pgo.windows.x64.checked.mch26,963,808+143
benchmarks.run_tiered.windows.x64.checked.mch10,507,436+157
coreclr_tests.run.windows.x64.checked.mch385,009,583+736
libraries.pmi.windows.x64.checked.mch59,506,418+190
libraries_tests.pmi.windows.x64.checked.mch121,311,207+220
realworld.run.windows.x64.checked.mch13,650,175+652
MinOpts (+383 bytes)
CollectionBase size (bytes)Diff size (bytes)
benchmarks.run_pgo.windows.x64.checked.mch10,473,053+73
benchmarks.run_tiered.windows.x64.checked.mch7,567,856+73
coreclr_tests.run.windows.x64.checked.mch269,424,110+237
FullOpts (+2,023 bytes)
CollectionBase size (bytes)Diff size (bytes)
aspnet.run.windows.x64.checked.mch29,278,684+156
benchmarks.run.windows.x64.checked.mch6,870,267+152
benchmarks.run_pgo.windows.x64.checked.mch16,490,755+70
benchmarks.run_tiered.windows.x64.checked.mch2,939,580+84
coreclr_tests.run.windows.x64.checked.mch115,585,473+499
libraries.pmi.windows.x64.checked.mch57,987,107+190
libraries_tests.pmi.windows.x64.checked.mch114,958,091+220
realworld.run.windows.x64.checked.mch12,500,933+652
@Ruihan-Yin
Ruihan-Yin
commented
Jun 29, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Fails should be irrelevant.
We had regression of about 2,400 bytes in the code size introduced by the changes. This should be expected as the regression is the conversion from VEX encoding to EVEX encoding when enabling embedded broadcast.

And there is no influence on the throughput.
Do we accept the results?
@tannergooding
tannergooding
commented
Jul 6, 2023

Copy link


Copy Markdown

Member
Do we accept the results?
The results look good/within reason to me. There are two important bits to call out...
First this change, in the typical case, trades a slight increase in code size (typically +2-bytes) for a decrease in data size (going from sizeof(VectorXXX<T>) to sizeof(T)) thereby increasing data locality and minimizing impact to the cache. This will often be a performance win.
Second, SPMI diffs are not currently tracking/including the data size allocations as part of "bytes of code" metric. HardwareIntrinsics.RayTracer.Packet256Tracer:Shade for example is +2 bytes of code, but -32 bytes of data so its actually a net win of -30 bytes of allocations
tannergooding
tannergooding
reviewed
Jul 6, 2023
View reviewed changes
Comment threadsrc/coreclr/jit/lowerxarch.cpp
Outdated

@tannergoodingtannergoodingJul 6, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This is better handled as

Suggested change

if (baseType == TYP_BYTE || baseType == TYP_UBYTE || baseType == TYP_SHORT || baseType == TYP_USHORT)
if (varTypeIsSmall(baseType))
@tannergooding
tannergooding
commented
Jul 6, 2023

Copy link


Copy Markdown

Member
CC. @dotnet/jit-contrib for secondary review.
@BruceForstall
BruceForstall
commented
Jul 6, 2023

Copy link


Copy Markdown

Contributor
superpmi-replay and superpmi-diffs pipelines are failing with messages like:
[17:40:21] ERROR: Couldn't load base metrics summary created by child process
[17:40:21] General fatal error

I wonder if the JIT is crashing (not asserting) with this PR?
@Ruihan-Yin
Ruihan-Yin
commented
Jul 6, 2023

Copy link


Copy Markdown

MemberAuthor
I wonder if the JIT is crashing (not asserting) with this PR?
First time seeing this fail in this PR, the recent changes might not cause the crashing, I will rebase the PR and re-run the CI again to confirm.
BruceForstall
BruceForstall
reviewed
Jul 7, 2023
View reviewed changes
Comment threadsrc/coreclr/jit/hwintrinsic.h
Outdated

@BruceForstallBruceForstallJul 6, 2023

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.

This seems like an odd location for this function. All the other functions here take a NamedIntrinsic. Should this instead be in emitxarch.h/cpp like IsMovInstruction or HasKMaskRegisterDest ?
@Ruihan-YinRuihan-YinJul 10, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

moved the function into emitxarch.h/cpp as a static function.
Comment threadsrc/coreclr/jit/instrsxarch.h
Outdated

@BruceForstallBruceForstallJul 7, 2023

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.

nit: no need to delete this empty line?
@tannergooding
tannergooding
commented
Jul 7, 2023

Copy link


Copy Markdown

Member
I wonder if the JIT is crashing (not asserting) with this PR?
There are similar failures in multiple PRs from what I've seen recently.
Noting that SPMI diffs is faulting in Python when processing the diffs summary:
Traceback (most recent call last):
File "C:\h\w\B2080989\p\superpmi.py", line 4667, in <module>
sys.exit(main(args))
File "C:\h\w\B2080989\p\superpmi.py", line 4558, in main
success = asm_diffs.replay_with_asm_diffs()
File "C:\h\w\B2080989\p\superpmi.py", line 2019, in replay_with_asm_diffs
self.write_asmdiffs_markdown_summary(write_fh, asm_diffs)
File "C:\h\w\B2080989\p\superpmi.py", line 2170, in write_asmdiffs_markdown_summary
write_row(*t)
File "C:\h\w\B2080989\p\superpmi.py", line 2165, in write_row
num_missed_base / num_contexts * 100,
ZeroDivisionError: division by zero

Replay is similarly failing with things like:
[17:42:50] Invoking: C:\h\w\B6DA095E\p\superpmi.exe -v ewi -r C:\h\w\B6DA095E\t\tmpdsiibugo\repro -p -jitoption JitStressRegs=0x80 -f C:\h\w\B6DA095E\t\tmpdsiibugo\coreclr_tests.run.windows.x64.checked.mch_fail.mcl -metricsSummary C:\h\w\B6DA095E\t\tmpdsiibugo\coreclr_tests.run.windows.x64.checked.mch_metrics.csv C:\h\w\B6DA095E\p\clrjit_win_x64_x64.dll C:\h\w\B6DA095E\p\artifacts\spmi\mch\02e334af-4e6e-4a68-9feb-308d3d2661bc.windows.x64\coreclr_tests.run.windows.x64.checked.mch
[17:42:50] ERROR: Couldn't load base metrics summary created by child process
[17:42:50] ERROR: Couldn't load base metrics summary created by child process
[17:42:50] ERROR: Couldn't load base metrics summary created by child process
[17:42:50] ERROR: Couldn't load base metrics summary created by child process
[17:42:50] General fatal error
[17:42:50] Running SuperPMI replay of C:\h\w\B6DA095E\p\artifacts\spmi\mch\02e334af-4e6e-4a68-9feb-308d3d2661bc.windows.x64\libraries.crossgen2.windows.x64.checked.mch

Did we recently get a JIT/EE version change and potentially not have up to date baselines?
@BruceForstall
BruceForstall
commented
Jul 7, 2023

Copy link


Copy Markdown

Contributor
Noting that SPMI diffs is faulting in Python when processing the diffs summary:
superpmi.py should gracefully handle that. But I think that's a symptom that superpmi.exe crashed.
Did we recently get a JIT/EE version change and potentially not have up to date baselines?
If the GUID changed, we wouldn't have this problem because we would only pick up matched MCH files.
If someone changed the interface without changing the GUID, then anything is possible.
@BruceForstall
BruceForstall
commented
Jul 7, 2023

Copy link


Copy Markdown

Contributor
I kicked off runs on main (not on a PR) so we can see if there are failures in the main branch:
https://dev.azure.com/dnceng-public/public/_build/results?buildId=331428&view=results
https://dev.azure.com/dnceng-public/public/_build/results?buildId=331429&view=results
@BruceForstall
BruceForstall
commented
Jul 7, 2023

Copy link


Copy Markdown

Contributor
@Ruihan-Yin The superpmi pipelines are now running clean. I would suggest to merge up to HEAD and re-push your PR to trigger re-run of these pipelines.
@Ruihan-Yin
Ruihan-Yin
commented
Jul 7, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
The superpmi pipelines are now running clean. I would suggest to merge up to HEAD and re-push your PR to trigger re-run of these pipelines.
Thanks for the notification, will resolve the reviews and rebase the branch soon.
@Ruihan-Yin
Enabled embedded broadcast for the following ops:
5ed01d7
and, andn, or, xor,
min, max,
div, mul, mull, sub,
variable shiftleftlogical/rightarithmetic/rightlogical
@Ruihan-Yin
 Bug fix:
0f746c4
 JIT used to use a uniform intrinsic for bitwise operations with all data
types, embedded broadcast is sensitive to input size in this case,
adding a helper to let emitter aware when input size is long/ulong.
@Ruihan-Yin
reset the instruction in the later phase
7825a18
when embedded broadcast is actually enabled
@Ruihan-Yin
filter irrelevant data type in embedded broadcast
1513e55
There are cases when broadcast node are falsely contained by a embedded
broadcast compatible node, while the data type is actually not supported
Adding extra logics to avoid this situation.
@Ruihan-Yin
update the condition on instruction reset:
a326e58
instructions with either long or ulong as basetype should be reset to
qword instructions.
@Ruihan-Yin
Apply format patch
4468b9f
@Ruihan-Yin
Resolve reviews:
89120d2
make the typecheck based on broadcast node it self.
@Ruihan-Yin
Resolve review:
f31da65
use `varTypeIsSmall` type check to cover all the unsupported data type
in embedded broadcast.
@Ruihan-Yin
Resolve reviews:
23c3f6e
1. put the IsBitwiseInstruction to a proper place.
2. nit: restored unnecessary line delete.
@Ruihan-Yin
Ruihan-Yin
commented
Jul 10, 2023

Copy link


Copy Markdown

MemberAuthor
Hi @BruceForstall, some tests are cancelled for some reasons, I suppose it is not relevant to the changes in this PR, shall I re-run the test again? And can you please review the new changes, if any. Thanks!
EgorBo
EgorBo
reviewed
Jul 14, 2023
View reviewed changes
Comment threadsrc/coreclr/jit/instr.cpp
 if (IsEmbBroadcast)
 {
 instOptions = INS_OPTS_EVEX_b;
 if (emitter::IsBitwiseInstruction(ins) && varTypeIsLong(op2->AsHWIntrinsic()->GetSimdBaseType()))
@EgorBoEgorBoJul 14, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

nit: do we need IsBitwiseInstruction at all since we have a switch over ins anyway? (we can just remove the unreached in debug
@tannergoodingtannergoodingJul 14, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Possibly not. I'll let Ruihan-Yin fix this in a follow up PR if they want to, that way we can land the important bit before the snap.
@Ruihan-YinRuihan-YinJul 14, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Thanks for the feedback, we will fix this point in the following PR on embedded broadcast.
@tannergooding
tannergooding
merged commit ef9a07c
into
dotnet:mainJul 14, 2023
@JulieLeeMSFTJulieLeeMSFT
mentioned this pull request
Aug 2, 2023

Closed

3 tasks
@BruceForstallBruceForstall
mentioned this pull request
Aug 3, 2023
 Merged

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


Reviewers

@tannergoodingtannergoodingtannergooding approved these changes
@EgorBoEgorBoEgorBo approved these changes
@BruceForstallBruceForstallAwaiting requested review from BruceForstall

Assignees


No one assigned


Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMIavx512Related to the AVX-512 architecturecommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@Ruihan-Yin@tannergooding@BruceForstall@EgorBo

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

Copy link


Copy Markdown

Member
This PR provides the embedded broadcast support for binary ops, to be more specific, ops using the genHWIntrinsic_R_R_RM path.
Including the following ops:

and, andn, or, xor,

min, max,

div, mul, mull, sub,

variable shiftleftlogical/rightarithmetic/rightlogical
@ghostghost
added

area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
community-contribution
Indicates that the PR has been added by a community member
labels
Jun 22, 2023
@ghost
ghost
commented
Jun 22, 2023

Copy link


Copy Markdown

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch

See info in area-owners.md if you want to be subscribed.
Issue Details
This PR provides the embedded broadcast support for binary ops, to be more specific, ops using the genHWIntrinsic_R_R_RM path.
Including the following ops:

and, andn, or, xor,

min, max,

div, mul, mull, sub,

variable shiftleftlogical/rightarithmetic/rightlogical
Author:Ruihan-Yin
Assignees:-
Labels:
area-CodeGen-coreclr
Milestone:-
@build-analysisbuild-analysisBot
mentioned this pull request
Jun 23, 2023

Closed

@Ruihan-YinRuihan-Yin
changed the title
Enabled embedded broadcast for binary opsJIT: Enabled embedded broadcast for binary opsJun 23, 2023
@Ruihan-YinRuihan-Yin
reopened this
Jun 23, 2023
@tannergoodingtannergooding
added
the 
avx512
Related to the AVX-512 architecture
label
Jun 27, 2023
@Ruihan-Yin
Ruihan-Yin
marked this pull request as ready for review
June 27, 2023 22:01
@Ruihan-Yin
Ruihan-Yin
commented
Jun 28, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Hi @tannergooding , the PR should be ready for review, would you please take a look at it?

it mostly includes:
  1. some updates on the metadata in the related tables to enable embedded broadcast in more instructions.
  2. some workaround on the bitwise instructions to make it work correctly with embedded broadcast when the input data type is in 64-bit.
  3. a bug fix where some broadcast nodes with data type less than 32 bits are accidentally contained, made some changes in the contain check to filter out these situations.
@build-analysisbuild-analysisBot
mentioned this pull request
Jun 29, 2023

Closed

tannergooding
tannergooding
reviewed
Jun 29, 2023
View reviewed changes
Comment threadsrc/coreclr/jit/instr.cpp
Outdated

@build-analysisbuild-analysisBot
mentioned this pull request
Jun 29, 2023

Closed

@Ruihan-Yin
Ruihan-Yin
commented
Jun 29, 2023

Copy link


Copy Markdown

MemberAuthor
windows x64
Diffs are based on 1,627,004 contexts (467,427 MinOpts, 1,159,577 FullOpts).
MISSED contexts: 2,227 (0.14%)
Overall (+2,406 bytes)
CollectionBase size (bytes)Diff size (bytes)
aspnet.run.windows.x64.checked.mch44,062,516+156
benchmarks.run.windows.x64.checked.mch7,244,558+152
benchmarks.run_pgo.windows.x64.checked.mch26,963,808+143
benchmarks.run_tiered.windows.x64.checked.mch10,507,436+157
coreclr_tests.run.windows.x64.checked.mch385,009,583+736
libraries.pmi.windows.x64.checked.mch59,506,418+190
libraries_tests.pmi.windows.x64.checked.mch121,311,207+220
realworld.run.windows.x64.checked.mch13,650,175+652
MinOpts (+383 bytes)
CollectionBase size (bytes)Diff size (bytes)
benchmarks.run_pgo.windows.x64.checked.mch10,473,053+73
benchmarks.run_tiered.windows.x64.checked.mch7,567,856+73
coreclr_tests.run.windows.x64.checked.mch269,424,110+237
FullOpts (+2,023 bytes)
CollectionBase size (bytes)Diff size (bytes)
aspnet.run.windows.x64.checked.mch29,278,684+156
benchmarks.run.windows.x64.checked.mch6,870,267+152
benchmarks.run_pgo.windows.x64.checked.mch16,490,755+70
benchmarks.run_tiered.windows.x64.checked.mch2,939,580+84
coreclr_tests.run.windows.x64.checked.mch115,585,473+499
libraries.pmi.windows.x64.checked.mch57,987,107+190
libraries_tests.pmi.windows.x64.checked.mch114,958,091+220
realworld.run.windows.x64.checked.mch12,500,933+652
@Ruihan-Yin
Ruihan-Yin
commented
Jun 29, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
Fails should be irrelevant.
We had regression of about 2,400 bytes in the code size introduced by the changes. This should be expected as the regression is the conversion from VEX encoding to EVEX encoding when enabling embedded broadcast.

And there is no influence on the throughput.
Do we accept the results?
@tannergooding
tannergooding
commented
Jul 6, 2023

Copy link


Copy Markdown

Member
Do we accept the results?
The results look good/within reason to me. There are two important bits to call out...
First this change, in the typical case, trades a slight increase in code size (typically +2-bytes) for a decrease in data size (going from sizeof(VectorXXX<T>) to sizeof(T)) thereby increasing data locality and minimizing impact to the cache. This will often be a performance win.
Second, SPMI diffs are not currently tracking/including the data size allocations as part of "bytes of code" metric. HardwareIntrinsics.RayTracer.Packet256Tracer:Shade for example is +2 bytes of code, but -32 bytes of data so its actually a net win of -30 bytes of allocations
tannergooding
tannergooding
reviewed
Jul 6, 2023
View reviewed changes
Comment threadsrc/coreclr/jit/lowerxarch.cpp
Outdated

@tannergoodingtannergoodingJul 6, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

This is better handled as

Suggested change

if (baseType == TYP_BYTE || baseType == TYP_UBYTE || baseType == TYP_SHORT || baseType == TYP_USHORT)
if (varTypeIsSmall(baseType))
@tannergooding
tannergooding
commented
Jul 6, 2023

Copy link


Copy Markdown

Member
CC. @dotnet/jit-contrib for secondary review.
@BruceForstall
BruceForstall
commented
Jul 6, 2023

Copy link


Copy Markdown

Contributor
superpmi-replay and superpmi-diffs pipelines are failing with messages like:
[17:40:21] ERROR: Couldn't load base metrics summary created by child process
[17:40:21] General fatal error

I wonder if the JIT is crashing (not asserting) with this PR?
@Ruihan-Yin
Ruihan-Yin
commented
Jul 6, 2023

Copy link


Copy Markdown

MemberAuthor
I wonder if the JIT is crashing (not asserting) with this PR?
First time seeing this fail in this PR, the recent changes might not cause the crashing, I will rebase the PR and re-run the CI again to confirm.
BruceForstall
BruceForstall
reviewed
Jul 7, 2023
View reviewed changes
Comment threadsrc/coreclr/jit/hwintrinsic.h
Outdated

@BruceForstallBruceForstallJul 6, 2023

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.

This seems like an odd location for this function. All the other functions here take a NamedIntrinsic. Should this instead be in emitxarch.h/cpp like IsMovInstruction or HasKMaskRegisterDest ?
@Ruihan-YinRuihan-YinJul 10, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

moved the function into emitxarch.h/cpp as a static function.
Comment threadsrc/coreclr/jit/instrsxarch.h
Outdated

@BruceForstallBruceForstallJul 7, 2023

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.

nit: no need to delete this empty line?
@tannergooding
tannergooding
commented
Jul 7, 2023

Copy link


Copy Markdown

Member
I wonder if the JIT is crashing (not asserting) with this PR?
There are similar failures in multiple PRs from what I've seen recently.
Noting that SPMI diffs is faulting in Python when processing the diffs summary:
Traceback (most recent call last):
File "C:\h\w\B2080989\p\superpmi.py", line 4667, in <module>
sys.exit(main(args))
File "C:\h\w\B2080989\p\superpmi.py", line 4558, in main
success = asm_diffs.replay_with_asm_diffs()
File "C:\h\w\B2080989\p\superpmi.py", line 2019, in replay_with_asm_diffs
self.write_asmdiffs_markdown_summary(write_fh, asm_diffs)
File "C:\h\w\B2080989\p\superpmi.py", line 2170, in write_asmdiffs_markdown_summary
write_row(*t)
File "C:\h\w\B2080989\p\superpmi.py", line 2165, in write_row
num_missed_base / num_contexts * 100,
ZeroDivisionError: division by zero

Replay is similarly failing with things like:
[17:42:50] Invoking: C:\h\w\B6DA095E\p\superpmi.exe -v ewi -r C:\h\w\B6DA095E\t\tmpdsiibugo\repro -p -jitoption JitStressRegs=0x80 -f C:\h\w\B6DA095E\t\tmpdsiibugo\coreclr_tests.run.windows.x64.checked.mch_fail.mcl -metricsSummary C:\h\w\B6DA095E\t\tmpdsiibugo\coreclr_tests.run.windows.x64.checked.mch_metrics.csv C:\h\w\B6DA095E\p\clrjit_win_x64_x64.dll C:\h\w\B6DA095E\p\artifacts\spmi\mch\02e334af-4e6e-4a68-9feb-308d3d2661bc.windows.x64\coreclr_tests.run.windows.x64.checked.mch
[17:42:50] ERROR: Couldn't load base metrics summary created by child process
[17:42:50] ERROR: Couldn't load base metrics summary created by child process
[17:42:50] ERROR: Couldn't load base metrics summary created by child process
[17:42:50] ERROR: Couldn't load base metrics summary created by child process
[17:42:50] General fatal error
[17:42:50] Running SuperPMI replay of C:\h\w\B6DA095E\p\artifacts\spmi\mch\02e334af-4e6e-4a68-9feb-308d3d2661bc.windows.x64\libraries.crossgen2.windows.x64.checked.mch

Did we recently get a JIT/EE version change and potentially not have up to date baselines?
@BruceForstall
BruceForstall
commented
Jul 7, 2023

Copy link


Copy Markdown

Contributor
Noting that SPMI diffs is faulting in Python when processing the diffs summary:
superpmi.py should gracefully handle that. But I think that's a symptom that superpmi.exe crashed.
Did we recently get a JIT/EE version change and potentially not have up to date baselines?
If the GUID changed, we wouldn't have this problem because we would only pick up matched MCH files.
If someone changed the interface without changing the GUID, then anything is possible.
@BruceForstall
BruceForstall
commented
Jul 7, 2023

Copy link


Copy Markdown

Contributor
I kicked off runs on main (not on a PR) so we can see if there are failures in the main branch:
https://dev.azure.com/dnceng-public/public/_build/results?buildId=331428&view=results
https://dev.azure.com/dnceng-public/public/_build/results?buildId=331429&view=results
@BruceForstall
BruceForstall
commented
Jul 7, 2023

Copy link


Copy Markdown

Contributor
@Ruihan-Yin The superpmi pipelines are now running clean. I would suggest to merge up to HEAD and re-push your PR to trigger re-run of these pipelines.
@Ruihan-Yin
Ruihan-Yin
commented
Jul 7, 2023•

edited

Loading

Copy link


Copy Markdown

MemberAuthor
The superpmi pipelines are now running clean. I would suggest to merge up to HEAD and re-push your PR to trigger re-run of these pipelines.
Thanks for the notification, will resolve the reviews and rebase the branch soon.
@Ruihan-Yin
Enabled embedded broadcast for the following ops:
5ed01d7
and, andn, or, xor,
min, max,
div, mul, mull, sub,
variable shiftleftlogical/rightarithmetic/rightlogical
@Ruihan-Yin
 Bug fix:
0f746c4
 JIT used to use a uniform intrinsic for bitwise operations with all data
types, embedded broadcast is sensitive to input size in this case,
adding a helper to let emitter aware when input size is long/ulong.
@Ruihan-Yin
reset the instruction in the later phase
7825a18
when embedded broadcast is actually enabled
@Ruihan-Yin
filter irrelevant data type in embedded broadcast
1513e55
There are cases when broadcast node are falsely contained by a embedded
broadcast compatible node, while the data type is actually not supported
Adding extra logics to avoid this situation.
@Ruihan-Yin
update the condition on instruction reset:
a326e58
instructions with either long or ulong as basetype should be reset to
qword instructions.
@Ruihan-Yin
Apply format patch
4468b9f
@Ruihan-Yin
Resolve reviews:
89120d2
make the typecheck based on broadcast node it self.
@Ruihan-Yin
Resolve review:
f31da65
use `varTypeIsSmall` type check to cover all the unsupported data type
in embedded broadcast.
@Ruihan-Yin
Resolve reviews:
23c3f6e
1. put the IsBitwiseInstruction to a proper place.
2. nit: restored unnecessary line delete.
@Ruihan-Yin
Ruihan-Yin
commented
Jul 10, 2023

Copy link


Copy Markdown

MemberAuthor
Hi @BruceForstall, some tests are cancelled for some reasons, I suppose it is not relevant to the changes in this PR, shall I re-run the test again? And can you please review the new changes, if any. Thanks!
EgorBo
EgorBo
reviewed
Jul 14, 2023
View reviewed changes
Comment threadsrc/coreclr/jit/instr.cpp
 if (IsEmbBroadcast)
 {
 instOptions = INS_OPTS_EVEX_b;
 if (emitter::IsBitwiseInstruction(ins) && varTypeIsLong(op2->AsHWIntrinsic()->GetSimdBaseType()))
@EgorBoEgorBoJul 14, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

nit: do we need IsBitwiseInstruction at all since we have a switch over ins anyway? (we can just remove the unreached in debug
@tannergoodingtannergoodingJul 14, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Possibly not. I'll let Ruihan-Yin fix this in a follow up PR if they want to, that way we can land the important bit before the snap.
@Ruihan-YinRuihan-YinJul 14, 2023

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Thanks for the feedback, we will fix this point in the following PR on embedded broadcast.
@tannergooding
tannergooding
merged commit ef9a07c
into
dotnet:mainJul 14, 2023
@JulieLeeMSFTJulieLeeMSFT
mentioned this pull request
Aug 2, 2023

Closed

3 tasks
@BruceForstallBruceForstall
mentioned this pull request
Aug 3, 2023
 Merged

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


Reviewers

@tannergoodingtannergoodingtannergooding approved these changes
@EgorBoEgorBoEgorBo approved these changes
@BruceForstallBruceForstallAwaiting requested review from BruceForstall

Assignees


No one assigned


Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMIavx512Related to the AVX-512 architecturecommunity-contributionIndicates that the PR has been added by a community member

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@Ruihan-Yin@tannergooding@BruceForstall@EgorBo