Automatically port System.Runtime.Intrinsics.X86 triple slash comments #3095

carlossanlop · 2019-08-27T20:43:02Z

This documentation is special. As discussed internally with @tannergooding and @CarolEidt , we want to port this documentation as is.
It will be useful for people already familiar with the instructions themselves or with the C++ intrinsics.

Here is the link to the System.Runtime.Intrinsics namespace documentation in our private build: https://review.docs.microsoft.com/en-us/dotnet/api/system.runtime.intrinsics.x86?view=netcore-3.0&branch=pr-en-us-3095

tannergooding · 2019-08-27T20:46:48Z

xml/System.Runtime.Intrinsics.X86/Avx2.xml

@@ -3642,7 +3652,10 @@
      </Parameters>
      <Docs>
        <param name="value">To be added.</param>
-        <summary>To be added.</summary>
+        <summary>
+          __m256i _mm256_cvtepu8_epi16 (__m128i a)


Will this render as two separate lines?

I don't think so, but if you look at other pre-existing summaries in this file, they have the exact same format: an endline at the beginning.

Actually, Docs doesn't seem to respect the endline in the middle. Take Avx.ConvertToVector256Int32 for example:
https://docs.microsoft.com/en-us/dotnet/api/system.runtime.intrinsics.x86.avx.converttovector256int32?view=netcore-3.0#System_Runtime_Intrinsics_X86_Avx_ConvertToVector256Int32_System_Runtime_Intrinsics_Vector256_System_Single__

At first I was assuming you were talking about lines 3655 and 3656, but the problem applies for both cases I mentioned above.

That's unfortunate; is there a way to ensure that the newline is retained?

Just asking for this case in particular. Ideally it would render as two separate lines; since the native instruction (second line) and the C++ intrinsic (first line) should be differentiated here.

There are two things to try:

Add two spaces in first line, before the line break.

If that doesn't work, use an HTML tag, either   or .

I think you need <para> tags to render as two lines.

I only have experience with  . I'll use that. I'll make sure all the pre-existing similar summaries have the same fix.

I did an example here: ea2cac4

It seemed to work: https://review.docs.microsoft.com/en-us/dotnet/api/System.Runtime.Intrinsics.X86.Avx2.ConvertToVector256Int16?branch=pr-en-us-3095&view=netcore-3.0#System_Runtime_Intrinsics_X86_Avx2_ConvertToVector256Int16_System_Runtime_Intrinsics_Vector128_System_Byte__

mairaw · 2019-08-28T02:20:30Z

xml/System.Runtime.Intrinsics.X86/Avx2.xml

+
+## Remarks
+
+The native signature does not exist. We provide this additional overload for completeness.


Suggestion to remove we from the sentence. If accepted, should be applied everywhere.

Also, will customers know what this means?

Suggested change

The native signature does not exist. We provide this additional overload for completeness.

The native signature doesn't exist. This additional overload is provided for completeness.

I applied the change. @tannergooding @CarolEidt, if a customer that is familiar with these APIs going to understand remarks like this?

Yes, I believe they will. The point is that, while most of these map to "native" C++ intrinsics, some do not. For example, these take an address (pointer), since the instruction supports an in-memory operand. The C++ intrinsics don't provide those as distinct overloads.

…to remarks or exception, depending on each case.

carlossanlop · 2019-08-29T21:35:34Z

Alright I got all the summaries and remarks fixed. This was... fun. Can you please take a look? I'd like to wait for the build to finish to ensure the   entity is rendered correctly as a forced space.
@mairaw @rpetrusha

rpetrusha · 2019-08-30T17:05:16Z

If your intention was that the line after the   should be indented because of the non-breaking spaces, @carlossanlop, that didn't work. Perhaps &nbsp?

carlossanlop · 2019-08-30T18:26:01Z

@rpetrusha I tried using   but Visual Studio marked that as an error, saying it's an unknown html entity. Let me try with one comment, wait for the build to finish, and see if it works.

carlossanlop · 2019-08-30T18:26:48Z

xml/System.Runtime.Intrinsics.X86/Sse2.xml

-            __m128i _mm_add_epi8 (__m128i a,  __m128i b)
-              PADDB xmm, xmm/m128
-            </summary>
+        <summary>__m128i _mm_add_epi8 (__m128i a,  __m128i b)<br />&#160;&#160;&#160;&#160;PADDB xmm, xmm/m128</summary>


I'm going to test   here, @rpetrusha.

Suggested change

<summary>__m128i _mm_add_epi8 (__m128i a, __m128i b)     PADDB xmm, xmm/m128</summary>

<summary>__m128i _mm_add_epi8 (__m128i a, __m128i b)     PADDB xmm, xmm/m128</summary>

Spaces will be ignored. Can we say instead Instruction: on the beginning of the 2nd line?

Also, I'd use <para> instead of   tags to make sure this will render as expected in VS. We'd have to test how
tags would be converted in the IntelliSense files and in VS.

M:System.Runtime.Intrinsics.X86.Sse2.Add

carlossanlop · 2019-09-04T16:47:28Z

@mairaw @rpetrusha although the build passed without warnings from the files I edited in this PR, there was no preview hyperlink for any of them.
I even tried accessing one of the review.microsoft.com links, but once I'm there, I cannot navigate to the System.Runtime.Intrinsics namespace because it doesn't exist. Do you know what happened? Should I trigger a new build?

https://opbuildstorageprod.blob.core.windows.net/report/2019%5C9%5C3%5C500c749d-de78-3d4a-00e4-269d599bf9ec%5CPullRequest%5C201909032106139666-3095%5Cworkflow_report.html?sv=2016-05-31&sr=b&sig=VHOHTNpTZ%2Fkywovm6DpQfYIyCSJS9yOBydHPZ40tB94%3D&st=2019-09-03T21%3A47%3A17Z&se=2019-10-04T21%3A52%3A17Z&sp=r

rpetrusha · 2019-09-04T17:36:31Z

Here is a link to the System.Runtime.Intrinsics.X86 namespace. All of the types included in the PR are accessible from it. I suspect that the difficulty you're having is with the version selector, @carlossanlop.

CarolEidt

This seems reasonable. I only looked at a sampling of the output at docs.microsoft.com, but it looks good to me.
Can you help me understand how this works? Are these now being auto-generated from the triple-slash comments in the code, and somehow the tool doing that has been adapted to insert the appropriate spaces and para elements?

carlossanlop · 2019-09-04T21:46:06Z

Thanks @rpetrusha for the link.

@mairaw you were absolutely right, even   is being ignored. I wanted to double check myself.

@CarolEidt @tannergooding since I cannot make Docs respect spacing, would you like me to use Maira's suggestion to add the prefix Instruction: at the beginning of every second line? For example:

__m128i _mm_add_epi8 (__m128i a,  __m128i b)
Instruction: PADDB xmm, xmm/m128

Or is this enough for you?:

__m128i _mm_add_epi8 (__m128i a,  __m128i b)
PADDB xmm, xmm/m128

@CarolEidt to answer your question: I ported the triple slash comments with my tool, then used a bunch of regular expressions in Visual Studio to substitute all the summaries in bulk and include <param> or  .

CarolEidt · 2019-09-05T00:34:38Z

Thanks @carlossanlop

I prefer the version without the Instruction: prefix. What do you think @tannergooding ?

carlossanlop · 2019-09-05T15:36:41Z

@CarolEidt I'll use the option without "instruction". It's what we used to have before anyway.

tannergooding · 2019-09-05T16:26:27Z

I agree, the version without Instruction is probably better.

tannergooding · 2019-09-05T16:31:24Z

@carlossanlop, is this only part of the changes?

There are several xml files that are missing (such as AVX, FMA, SSE3, etc)

carlossanlop · 2019-09-05T16:39:53Z

@carlossanlop, is this only part of the changes?

There are several xml files that are missing (such as AVX, FMA, SSE3, etc)

@tannergooding I am not sure if you mean that I missed to port triple slash comments or if I missed adding <param> to existing comments. Since I didn't find any additional triple slash comments to port in the files you just mentioned, I am going to assume you mean I missed adding <param> to certain files. You're correct, I didn't catch them. I only modified files that had comments to be automatically ported, but I can submit another PR fixing those, so we can get the automatically ported comments merged first.

carlossanlop · 2019-09-05T16:46:25Z

@mairaw @rpetrusha I addressed all the comments. If the build passes without warnings, I think we can get this merged.

tannergooding · 2019-09-05T16:54:41Z

@carlossanlop, I meant that there are 5 files being modified in this PR; but more like 24 files that contain the intrinsic documentation.

The other 19 files don't have some of the fixes being made in this PR; such as breaking up:

__m128 _mm_add_ps (__m128 a, __m128 b) ADDPS xmm, xmm/m128

into

__m128 _mm_add_ps (__m128 a, __m128 b)
ADDPS xmm, xmm/m128

carlossanlop · 2019-09-05T16:56:50Z

@carlossanlop, I meant that there are 5 files being modified in this PR; but more like 24 files that contain the intrinsic documentation.

The other 19 files don't have some of the fixes being made in this PR; such as breaking up:
__m128 _mm_add_ps (__m128 a, __m128 b) ADDPS xmm, xmm/m128
into
__m128 _mm_add_ps (__m128 a, __m128 b)
ADDPS xmm, xmm/m128

Yes, I'm taking care of them in a separate PR to enclose those instructions with param.

carlossanlop · 2019-09-05T17:36:39Z

I have the PR ready for the rest of the files, @tannergooding, but that branch's parent is this one (I wanted to avoid duplicating changes when I bulk replaced with regex). So I'll wait for this PR to get merged, rebase that new branch to master, and then submit the new PR.
@nevermind, I was able to cherry-pick my change to a new branch, without adding the files already being modified in this PR. Please take a look: #3125

rpetrusha

This looks good, I think. If you're satisfied with it, is it ready to merge, @carlossanlop?

carlossanlop · 2019-09-06T00:32:27Z

@rpetrusha Yes, please merge it. Thanks!

Automatically port System.Runtime.Intrinsics.X86

3fee0a7

carlossanlop added new-content Indicates PRs that contain new articles waiting-on-reviews Indicates PRs that cannot be merged because of the lack of reviews 🏁 Release: .NET Core 3.0 :checkered_flag: Release: .NET Core 3.0 labels Aug 27, 2019

carlossanlop added this to the August 2019 milestone Aug 27, 2019

carlossanlop requested review from CarolEidt, tannergooding, rpetrusha and mairaw August 27, 2019 20:43

carlossanlop self-assigned this Aug 27, 2019

tannergooding reviewed Aug 27, 2019

View reviewed changes

mairaw added 5 commits August 27, 2019 19:12

remove BOM character

a09ef2d

remove BOM character

6897d91

remove BOM character

de1a462

remove BOM character

e1a2a93

test para tags

ea2cac4

mairaw reviewed Aug 28, 2019

View reviewed changes

Forced endline and spacing in function summaries. Moved explanations …

a63a0f5

…to remarks or exception, depending on each case.

carlossanlop force-pushed the Intrinsics branch from 8bc5d6b to a63a0f5 Compare August 29, 2019 21:33

CarolEidt approved these changes Aug 29, 2019

View reviewed changes

carlossanlop commented Aug 30, 2019

View reviewed changes

carlossanlop and others added 2 commits August 30, 2019 11:27

Test nbsp to force space

315646d

M:System.Runtime.Intrinsics.X86.Sse2.Add

Testing <para> and  

01a4187

CarolEidt approved these changes Sep 4, 2019

View reviewed changes

Remove  , leave only <para></para>

ec85bc0

carlossanlop added verify-build-before-merge and removed waiting-on-reviews Indicates PRs that cannot be merged because of the lack of reviews labels Sep 5, 2019

rpetrusha approved these changes Sep 5, 2019

View reviewed changes

mairaw merged commit e4ae8e2 into dotnet:master Sep 6, 2019

mairaw removed the verify-build-before-merge label Sep 6, 2019

carlossanlop deleted the Intrinsics branch November 6, 2019 18:58


		## Remarks

		The native signature does not exist. We provide this additional overload for completeness.

	The native signature does not exist. We provide this additional overload for completeness.
	The native signature doesn't exist. This additional overload is provided for completeness.

	<summary>__m128i _mm_add_epi8 (__m128i a, __m128i b)<br />    PADDB xmm, xmm/m128</summary>
	<summary>__m128i _mm_add_epi8 (__m128i a, __m128i b)<br />    PADDB xmm, xmm/m128</summary>

Automatically port System.Runtime.Intrinsics.X86 triple slash comments #3095

Automatically port System.Runtime.Intrinsics.X86 triple slash comments #3095

Uh oh!

Conversation

carlossanlop commented Aug 27, 2019 • edited by rpetrusha Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlossanlop Aug 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlossanlop Aug 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlossanlop commented Aug 29, 2019

Uh oh!

rpetrusha commented Aug 30, 2019

Uh oh!

carlossanlop commented Aug 30, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlossanlop commented Sep 4, 2019

Uh oh!

rpetrusha commented Sep 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CarolEidt left a comment

Choose a reason for hiding this comment

Uh oh!

carlossanlop commented Sep 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CarolEidt commented Sep 5, 2019

Uh oh!

carlossanlop commented Sep 5, 2019

Uh oh!

tannergooding commented Sep 5, 2019

Uh oh!

tannergooding commented Sep 5, 2019

Uh oh!

carlossanlop commented Sep 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carlossanlop commented Sep 5, 2019

Uh oh!

tannergooding commented Sep 5, 2019

Uh oh!

carlossanlop commented Sep 5, 2019

Uh oh!

carlossanlop commented Sep 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carlossanlop commented Aug 27, 2019 •

edited by rpetrusha

Loading

carlossanlop Aug 27, 2019 •

edited

Loading

carlossanlop Aug 27, 2019 •

edited

Loading

rpetrusha commented Sep 4, 2019 •

edited

Loading

carlossanlop commented Sep 4, 2019 •

edited

Loading

carlossanlop commented Sep 5, 2019 •

edited

Loading

carlossanlop commented Sep 5, 2019 •

edited

Loading