Add 'w' and 's' bit to xarch instruction flags. #61198

anthonycanino · 2021-11-04T08:40:45Z

This PR addresses #35305.

Changes

Change encodes 'w' and 's' in the insFlags struct and INS_FLAG entry for xarch instruction table. In addition, HasWBit and HasSBit check if this flag is set for an instruction, which allows to start simplifying some of the various ad-hoc checks for these bits that were previously done per-instruction throughout emitxarch.cpp.

Discussion

Please see #35305 for an extended discussion on adding the 'w' and 's' bit as INS_FLAGS entries.

There is one point that I need some expert knowledge on: whether or not we need isInsCMOV and !isInsCMOV at two different points. I left comments in the code to draw attention to it: 33a9ddb#diff-6b4e0f32449f2f144e05699f59f74415a564693637f643084a896dfbd081830dR10984 and 33a9ddb#diff-6b4e0f32449f2f144e05699f59f74415a564693637f643084a896dfbd081830dR11449

ghost · 2021-11-04T08:40:52Z

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

Issue Details

This PR addresses #35305.

Changes

Change encodes 'w' and 's' in the insFlags struct and INS_FLAG entry for xarch instruction table. In addition, HasWBit and HasSBit check if this flag is set for an instruction, which allows to start simplifying some of the various ad-hoc checks for these bits that were previously done per-instruction throughout emitxarch.cpp.

Discussion

Please see #35305 for an extended discussion on adding the 'w' and 's' bit as INS_FLAGS entries.

There is one point that I need some expert knowledge on: whether or not we need isInsCMOV and !isInsCMOV at two different points. I left comments in the code to draw attention to it: 33a9ddb#diff-6b4e0f32449f2f144e05699f59f74415a564693637f643084a896dfbd081830dR10984 and 33a9ddb#diff-6b4e0f32449f2f144e05699f59f74415a564693637f643084a896dfbd081830dR11449

Author:	anthonycanino
Assignees:	-
Labels:	`area-CodeGen-coreclr`, `community-contribution`
Milestone:	-

JulieLeeMSFT · 2021-11-04T18:37:56Z

@tannergooding PTAL the community PR.

anthonycanino · 2021-11-08T15:27:25Z

I believe the Linux x64 test Interop/PInvoke/Generics/GenericsTest/GenericsTest.sh currently failing on main (#61300)

src/coreclr/jit/emitxarch.cpp

tannergooding · 2021-11-10T19:49:56Z

The changes generally LGTM. I still need to finish going through the instruction tables to validate the instrsxarch.h changes all look good.

tannergooding · 2021-11-15T19:49:28Z

src/coreclr/jit/emitxarch.cpp

+//
+// Return Value:
+//    true if instruction has the 's' bit, false otherwise
+bool emitter::HasSBit(instruction ins)


For s in particular, I wonder if there needs to be a specialization of it...

For example, if we look at

We'll see that all variants have the w bit; but only 2 variants has the s bit (and have the same pattern). There is an alternative/shorter encoding that does immediate to AL, AX, or EAX which doesn't use the s bit.

Likewise if we look at the 64-bit mode variants:

There are similar cases for the w/s-bit and so "just the flag" doesn't look sufficient to know "does the bit exist and is it valid to set"

The annotations otherwise all look correct for where some encoding for an instruction uses the s and or w bits, respectively.

So do you think that we need a combination of looking at what form the instruction is in and whether it has the w/s bit to properly implement HasSBit and HasWBit?

That's somewhat the concern, yes.

Basically, simply annotating Has_Sbit and Has_Wbit isn't enough and might cause later downstream bugs. It really is a few factors that determine if the S/W bit are present and if it means anything.

Ok, I understand.

I have to go back over the tables to understand how many cases there are that might actually arise, or if there is a general enough pattern that most of the logic can be coded in using the Has_Sbit and Has_Wbit + checks in HasSbit and HasWbit can be enough, or whether we to specialize the flag encoding further?

Do you have any thoughts on the path forward? I notice that a lot of the cases that do not have s and w bit in 64-bit mode are when Rex.W is set (which I originally thought indicates that 'w' bit is not needed).

I think that "w" in the legend for "The value of bit W. in REX has no effect" is actually referring to this, and not the 'w' bit that we have been discussing...

because to my understanding, in 64-bit mode, the operands are always 64-bit for the call instruction and the REX.W can be 0 or 1, but it won't change the operand size for this particular instruction.

The "S" in the legend for "If the value of REX.W. is 1, it overrides the presence of 66H" is used like so

In other words, those 'S' and 'w' legends are not referring to the 's' and 'w' bits. So your initial understanding of "there is a 32-bit instruction form (taking potential operand override prefixes) by setting the lowest bit" is correct.

Shall I update the comments to make this more clear?

Thanks, that explains it.
It would be great if you could expand the comments on HasWBit and HasSBit to include an explanation and perhaps point to the table in the Intel manual. We could also consider something more descriptive such as HasRegularWideForm/HasRegularWideImmediateForm with a pointer to this being the w/s bits in the encodings from that section in the Intel manual. But I will leave this up to you -- I don't have a strong opinion as long as it is explained how to interpret the return value.

I added some more comments and renamed to HasRegularWideForm. Happy to adjust. Do the comments make it clear what is happening here with respect to the instruction needing the 'w' bit set IF it is in a form where the 'w' bit is present?

I think this is where further refactoring will help per.

I don't think the comments explain much. I would add something like:

// Many x86/x64 instructions follow a regular encoding scheme where the // byte-sized version of an instruction has the lowest bit of the opcode cleared // while the 32-bit version of the instruction (taking potential prefixes to // override operand size) has the lowest bit set. This function returns true if // the instruction follows this format. // Note that this bit is called `w` in the encoding table in Section B.2 of // Volume 2 of the Intel Architecture Software Developer Manual.

and

// As above, many instructions taking immediates have a regular form used to // encode whether the instruction takes a sign-extended 1-byte immediate or a // (in 64-bit sign-extended) 4-byte immediate, by respectively setting and // clearing the second lowest bit. This bit is called the s bit in table B.2.

for the other one.

Thanks. I've gone ahead and updated the comments. Mostly copied what you pasted into the format in the file. Does it look good?

Change encodes 'w' and 's' in the insFlags struct and INS_FLAG entry for xarch instruction table. In addition, `HasWBit` and `HasSBit` check if this flag is set for an instruction, which allows to start simplifying some of the various ad-hoc checks for these bits that were previously done per-instruction throughout emitxarch.cpp.

Co-authored-by: Tanner Gooding <tagoo@outlook.com>

jakobbotsch · 2021-12-13T18:42:15Z

The CI asmdiffs leg shows no diffs: https://dev.azure.com/dnceng/public/_build/results?buildId=1499327&view=ms.vss-build-web.run-extensions-tab
Let me run the fuzzers.

/azp run Antigen, Fuzzlyn

jakobbotsch · 2021-12-13T18:44:17Z

/azp run Antigen, Fuzzlyn

azure-pipelines · 2021-12-13T18:44:43Z

Azure Pipelines successfully started running 2 pipeline(s).

jakobbotsch · 2021-12-14T12:03:32Z

The Fuzzlyn failures are known. @kunalspathak, can you look if the Antigen failures are known?

kunalspathak · 2021-12-14T17:44:06Z

The Fuzzlyn failures are known. @kunalspathak, can you look if the Antigen failures are known?

Yes, all failures are known failures.

jakobbotsch · 2021-12-15T10:18:46Z

src/coreclr/jit/emitxarch.cpp

-        if ((size != EA_1BYTE) && (ins != INS_imul) && (ins != INS_bsf) && (ins != INS_bsr) && (!insIsCMOV(ins)) &&
-            !IsSSEInstruction(ins) && !IsAVXInstruction(ins))
+        // Anthony: I believe we may remove !insIsCMOV check, but leaving for comparison purposes with another
+        // similar check below


Nits: Seems like the TODO above is outdated with this change, remove it?
Also, I would prefer to not leave author names in comments and to write them in third person or plural first person instead.

Thanks, I have removed the todo and my comment.

jakobbotsch · 2021-12-15T10:38:59Z

src/coreclr/jit/emitxarch.cpp

-             insIsCMOV(ins)) &&
-            size != EA_1BYTE)
+        // Anthony: Per L10986, why is this insIsCMOV (which does not have a w bit)
+        // and above is !insIsCMOV


I can't say exactly why these checks are there, but since we currently do not generate cmov I would be fine with removing the checks and leave it up to future work to make sure the emitter supports them if we decide to start generating it again.

Ok, I have gone ahead and removed the comments and the redundant cmov checks.

jakobbotsch · 2021-12-18T18:21:24Z

Thanks!

dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Nov 4, 2021

ghost added the community-contribution Indicates that the PR has been added by a community member label Nov 4, 2021

anthonycanino closed this Nov 4, 2021

runfoapp bot mentioned this pull request Nov 4, 2021

Linker tests failing with no space left on device on Linux x64 #60927

Closed

anthonycanino reopened this Nov 4, 2021

anthonycanino force-pushed the anthony/68 branch from 33a9ddb to cdd0673 Compare November 4, 2021 17:05

JulieLeeMSFT requested a review from tannergooding November 4, 2021 18:36

JulieLeeMSFT assigned tannergooding and anthonycanino Nov 4, 2021

JulieLeeMSFT added this to the 7.0.0 milestone Nov 4, 2021

tannergooding reviewed Nov 10, 2021

View reviewed changes

src/coreclr/jit/emitxarch.cpp Outdated Show resolved Hide resolved

tannergooding reviewed Nov 15, 2021

View reviewed changes

anthonycanino and others added 3 commits December 6, 2021 11:30

Run jit-format to fix formatting errors.

2edfb44

Update src/coreclr/jit/emitxarch.cpp

c30663b

Co-authored-by: Tanner Gooding <tagoo@outlook.com>

anthonycanino force-pushed the anthony/68 branch from bb4e629 to c30663b Compare December 6, 2021 20:16

jakobbotsch reviewed Dec 15, 2021

View reviewed changes

anthonycanino added 3 commits December 15, 2021 09:22

Remove TODOs and insIsCMOV checks.

f38a738

Rename HasWBit and HasSBit, added comments for clarity.

f8ed360

Updated HasRegularWideForm / HasRegularWideImmediateForm comments.

9fdd08a

jakobbotsch approved these changes Dec 17, 2021

View reviewed changes

jakobbotsch merged commit 3424923 into dotnet:main Dec 18, 2021

ghost locked as resolved and limited conversation to collaborators Jan 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add 'w' and 's' bit to xarch instruction flags. #61198

Add 'w' and 's' bit to xarch instruction flags. #61198

anthonycanino commented Nov 4, 2021

ghost commented Nov 4, 2021

Changes

Discussion

JulieLeeMSFT commented Nov 4, 2021

anthonycanino commented Nov 8, 2021

tannergooding commented Nov 10, 2021

tannergooding Nov 15, 2021

tannergooding Nov 15, 2021

anthonycanino Nov 15, 2021

tannergooding Nov 15, 2021

anthonycanino Nov 15, 2021

anthonycanino Dec 16, 2021

jakobbotsch Dec 16, 2021 •

edited

Loading

anthonycanino Dec 17, 2021

jakobbotsch Dec 17, 2021 •

edited

Loading

anthonycanino Dec 17, 2021

jakobbotsch commented Dec 13, 2021

jakobbotsch commented Dec 13, 2021

azure-pipelines bot commented Dec 13, 2021

jakobbotsch commented Dec 14, 2021

kunalspathak commented Dec 14, 2021

jakobbotsch Dec 15, 2021

anthonycanino Dec 15, 2021

jakobbotsch Dec 15, 2021

anthonycanino Dec 15, 2021

jakobbotsch commented Dec 18, 2021

Add 'w' and 's' bit to xarch instruction flags. #61198

Add 'w' and 's' bit to xarch instruction flags. #61198

Conversation

anthonycanino commented Nov 4, 2021

Changes

Discussion

ghost commented Nov 4, 2021

Changes

Discussion

JulieLeeMSFT commented Nov 4, 2021

anthonycanino commented Nov 8, 2021

tannergooding commented Nov 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakobbotsch Dec 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakobbotsch Dec 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakobbotsch commented Dec 13, 2021

jakobbotsch commented Dec 13, 2021

azure-pipelines bot commented Dec 13, 2021

jakobbotsch commented Dec 14, 2021

kunalspathak commented Dec 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakobbotsch commented Dec 18, 2021

jakobbotsch Dec 16, 2021 •

edited

Loading

jakobbotsch Dec 17, 2021 •

edited

Loading