Refactoring emitInsBinary by tannergooding · Pull Request #15836 · dotnet/coreclr

tannergooding · 2018-01-12T02:32:34Z

Resolves https://github.com/dotnet/coreclr/issues/15829

tannergooding · 2018-01-12T02:32:47Z

FYI. @CarolEidt, @mikedn, @dotnet/jit-contrib

tannergooding · 2018-01-12T02:38:15Z

+
+                default: // Addressing mode [regBase + regIndex * Scale + const]
+                {
+                    instrDesc* id = nullptr;


I want to refactor this code separately (as part of https://github.com/dotnet/coreclr/issues/15828).

After this is refactored, we could also rename the method to genInsBinary and move it to Codegen, which may be a more appropriate place.

tannergooding · 2018-01-12T02:43:46Z

-    // src can be anything but both src and dst cannot be addr modes
-    // or at least cannot be contained addr modes
-    if (dst->isUsedFromMemory())
+    if (dst->isContained() || (dst->isLclField() && (dst->gtRegNum == REG_NA)) || dst->isUsedFromSpillTemp())


I wasn't sure why dst isn't marked as contained for (dst->isLclField() && (dst->gtRegNum == REG_NA)) (see here for the old code), it seems to map to a GT_STORE_LCL_FLD and, being a store, I would have expected it to be containable...

I'm not sure about that either.

Hmm, a contained dst is likely rare in this code path. This might worth a separate investigation to avoid surprises.

tannergooding · 2018-01-12T02:47:37Z

+        if (memOp->isIndir())
+        {
+            GenTreeIndir* memIndir = memOp->AsIndir();
+            GenTree*      memBase  = memIndir->gtOp1;


I kept the old name here, but I don't think memBase is appropriate.

There is also memIndir->Base() which returns something slightly different and which is used in emitHandleMemOp

tannergooding · 2018-01-12T05:16:51Z

-        memBase = mem->gtOp1;
+        if (src->IsCnsIntOrI() || src->IsCnsFltOrDbl())
+        {
+            cnsOp = src;


I originally had a assert(!src->isUsedFromMemory() || src->IsCnsFltOrDbl()); right above this. However, it appears that on x86 Checked builds, we will sometimes spill a GT_CNS_INT to the stack.

See the failure here which reported:

Assert failure(PID 4672 [0x00001240], Thread: 4644 [0x1224]): Assertion failed '!src->isUsedFromMemory() || src->IsCnsFltOrDbl()' in 'System.Diagnostics.Tracing.EventProvider:WriteEvent(byref,int,int,int,ref):bool:this' (IL size 1225)

I was not seeing the same failure in x86 Debug or Release.

Looks like x86 Checked is still failing due to the stack spill. Will investigate sometime tomorrow.

isUsedFromSpillTemp needed to take precedence over everything else (including IsCns and IsIndir).

mikedn · 2018-01-12T06:10:13Z

Hmm, I have to take a closer look when I have a bit more spare time. I had something a bit different in mind, it included moving this out of emitter to CodeGen. I wonder if I kept the branch where I experimented with this...

tannergooding · 2018-01-12T06:18:50Z

it included moving this out of emitter to CodeGen

I mentioned doing that above, but think it should wait until after the address mode refactoring (https://github.com/dotnet/coreclr/issues/15828) happens (so that the emitter specific code can be contained to the emitter)

CarolEidt

LGTM Would love to get another review from @dotnet/jit-contrib

CarolEidt · 2018-01-12T23:14:13Z

-    // src can be anything but both src and dst cannot be addr modes
-    // or at least cannot be contained addr modes
-    if (dst->isUsedFromMemory())
+    if (dst->isContained() || (dst->isLclField() && (dst->gtRegNum == REG_NA)) || dst->isUsedFromSpillTemp())


I'm not sure about that either.

CarolEidt · 2018-01-12T23:14:49Z

-    if (dst->isUsedFromMemory())
+    if (dst->isContained() || (dst->isLclField() && (dst->gtRegNum == REG_NA)) || dst->isUsedFromSpillTemp())
    {
+        // dst can only be a reg or modrm


At this point it can only be memory, given the condition above.

CarolEidt · 2018-01-17T23:22:28Z

ping @dotnet/jit-contrib - This is broadly used for xarch codegen, so I think it would be good to get a second review on it. Any takers?

tannergooding · 2018-01-18T17:15:43Z

@CarolEidt, which additional jobs should be run to validate this?

I imagine we want to run all or most pri-1 tests, before merging, to get the additional validation (given the broad use).

mikedn · 2018-01-18T17:19:17Z

+    // We can only have one memory operand and only src can be a constant operand
+    GenTree* memOp = nullptr;
+    GenTree* cnsOp = nullptr;
+    GenTree* otrOp = nullptr;


I guess otr is shorthand for other?

Yes (happy to change if its confusing, I forgot I named it this).

I vote for a rename (IMO, "otherOp" is fine; no need for all vars to be 5 characters long)

fixed to be otherOp

mikedn · 2018-01-18T17:31:15Z

-        tmpDsc = codeGen->getSpillTempDsc(src);
-    }
-    else if (dst->isUsedFromSpillTemp())
+    if (memOp != nullptr)


It might have been better to first handle the simple cases to get those out of the way quicker. As is now you have to scroll through all the memOp case to reach the rest.

mikedn · 2018-01-18T17:36:40Z

+
+                case GT_LCL_VAR:
+                {
+                    assert(memOp->IsRegOptional() || !emitComp->lvaTable[memOp->gtLclVar.gtLclNum].lvIsRegCandidate());


It would be better to split this assert in two. The second condition should be moved after the assignment below so it can reuse varNum.

It's an || condition, so it can't be split... No matter how you do the memOp->IsRegOptional() check, it can fail on its own.

Unless I'm just too tired to remember how to do this 😄

Oops, missed the ||.

AndyAyersMS · 2018-01-18T18:12:36Z

If this is a pure refactoring, then you should verify the result is no-diff.

You can use jit-diffs for this in Core but I'd also recommend running the SPMI based diffs over in desktop as they give better all around coverage of the various jit modes (jit-diffs will only test R2R codegen).

tannergooding · 2018-01-18T18:15:10Z

If this is a pure refactoring, then you should verify the result is no-diff.

Is there a doc on how to do this for Desktop? Is there an easy way to do this 'en-masse' for Core?

AndyAyersMS · 2018-01-18T18:23:19Z

Jit-diff is described here. I generally do this with two enlistments, one built vs master, the other vs my change. You need both check and release builds.

For desktop I can send you pointers via email.

CarolEidt · 2018-01-18T18:34:07Z

Thanks @AndyAyersMS - I think that doing both coreclr and desktop diffs would be ideal validation (and is what I usually do for refactoring changes).

tannergooding · 2018-01-18T18:39:16Z

Rebased onto HEAD for the jit-diff tests.

BruceForstall · 2018-01-18T19:25:37Z

(This is a poster child for CI-triggered all-platform all-architecture asm diffs, including superpmi asm diffs, which was once a goal but is not being worked on currently.)

BruceForstall

Make sure you trigger a bunch of x86/x64 stress mode jobs.
Seems like there could be more comments, maybe including examples. I guess there weren't many before.

BruceForstall · 2018-01-18T19:51:06Z

@@ -2920,174 +2920,283 @@ void emitter::emitInsStoreLcl(instruction ins, emitAttr attr, GenTreeLclVarCommo

 regNumber emitter::emitInsBinary(instruction ins, emitAttr attr, GenTree* dst, GenTree* src)


Since you've got this function in your head now, it would be useful to write a good (standard format) function header comment describing it.

Added a function header.

BruceForstall · 2018-01-18T19:51:57Z

+    // We can only have one memory operand and only src can be a constant operand
+    GenTree* memOp = nullptr;
+    GenTree* cnsOp = nullptr;
+    GenTree* otrOp = nullptr;


I vote for a rename (IMO, "otherOp" is fine; no need for all vars to be 5 characters long)

mikedn · 2018-01-18T21:42:06Z

which also brings about a shrinking of the Lcl frame size

Are you sure that the diffs you are doing have a proper baseline or whatever is what they are using? It seems strange for any change to emitInsBinary to have such an effect.

tannergooding · 2018-01-18T21:49:41Z

Are you sure that the diffs you are doing have a proper baseline or whatever is what they are using? It seems strange for any change to emitInsBinary to have such an effect.

Looks like emitxarch had some extra changes in it that aren't in the current TFS baseline. I'll see if I can get it back to just the emitInsBinary changes

mikedn · 2018-01-18T21:54:32Z

FWIW I've got your branch and run jit-diff, there are no changes.

AndyAyersMS · 2018-01-18T22:17:40Z

Ah, I should have mentioned that you need to be careful about ensuring that your TFS enlistment is synced to the same point as your GitHub enlistment. They are usually close but the mirroring can lag or get blocked at times.

tannergooding · 2018-01-18T22:47:11Z

Added a function header, some more detailed comments, and changed otrOp to otherOp.

tannergooding · 2018-01-18T23:03:03Z

Ok. No diffs for CoreCLR (tested on Windows against x64 and x86 for Checked and Released builds).

I think I'm doing something wrong for the Desktop diffs and am going to follow up to see if I can figure it out.

tannergooding · 2018-01-19T03:58:39Z

                    offset = 0;

                    // Ensure that all the GenTreeIndir values are set to their defaults.
-                    assert(memBase->gtRegNum == REG_NA);


Assert was incorrect. Under JitStress=1 on Windows x64 Checked we had a few tests fail.

Validated that we still want to go the emit_R_S or emit_S_R route by checking the code-diff.

tannergooding · 2018-01-19T04:03:56Z

Okay:

No diffs on CoreCLR
No diffs for SuperPMI on Desktop.
There was an incorrect assert that was hit under JitStress1 that I removed.
I am running the Desktop DDR suite for validation as well.

tannergooding · 2018-01-19T08:28:06Z

Most of the Windows JITStress jobs are failing the eventsourcetrace test, which looks new and looks like it is supposed to be skipped in most places (#15917)

tannergooding · 2018-01-19T15:34:11Z

The only non-eventsourcetrace test failures I see are in x86 Checked jitstressregs8, which also is failing the non-pr jobs with the same (farthestRefPhysRegRecord != nullptr)

tannergooding · 2018-01-19T16:25:57Z

https://github.com/dotnet/coreclr/issues/15924 is tracking the eventsourcetrace issue.

tannergooding · 2018-01-19T19:01:24Z

I am running the Desktop DDR suite for validation as well.

This finished.

I had a few failures with a failure type of Deterministic 3:NoBaseline. (I think these can be ignored, based on the failure type)

There were also no diffs (I did have a network outage that failed the amd64chk diff run, as it couldn't find the file share for the baseline, but I had already ran these manually beforehand and saw no failures).

tannergooding · 2018-01-19T19:04:57Z

@CarolEidt, @AndyAyersMS, @BruceForstall

Is there any additional validation needed?

As a summary (assuming I read all the logs correctly):

No diffs on CoreCLR or Desktop
Only test failures on Desktop look to be for Deterministic 3:NoBaseline
JitStress failures on CoreCLR are due to an external issue and only impact a single test(https://github.com/dotnet/coreclr/issues/15924)
- The x86 JitStressRegs=8 had failures matching the runs against master (farthestRefPhysRegRecord != nullptr)

CarolEidt · 2018-01-19T19:25:37Z

The only non-eventsourcetrace test failures I see are in x86 Checked jitstressregs8, which also is failing the non-pr jobs with the same (farthestRefPhysRegRecord != nullptr)

Opened #15932

I don't see the need for any further validation.

tannergooding · 2018-01-19T21:33:11Z

Is there any other feedback, or does this look good to merge (@AndyAyersMS, @BruceForstall, @mikedn)?

BruceForstall

LGTM. Thanks for the additional comments.

tannergooding · 2018-01-19T23:51:48Z

Merging.

tannergooding commented Jan 12, 2018

View reviewed changes

jkotas added the area-CodeGen label Jan 12, 2018

tannergooding commented Jan 12, 2018

View reviewed changes

This was referenced Jan 12, 2018

Adding basic containment support to the x86 HWIntrinsics #15804

Merged

Enable CORINFO_INTRINSIC Round, Ceiling, and Floor to generate ROUNDSS and ROUNDSD #14736

Merged

CarolEidt approved these changes Jan 16, 2018

View reviewed changes

mikedn reviewed Jan 18, 2018

View reviewed changes

BruceForstall reviewed Jan 18, 2018

View reviewed changes

Refactoring emitInsBinary

a191099

Removing an incorrect assert from emitInsBinary

9de5dcd

tannergooding commented Jan 19, 2018

View reviewed changes

BruceForstall approved these changes Jan 19, 2018

View reviewed changes

tannergooding merged commit 9afce33 into dotnet:master Jan 19, 2018

tannergooding deleted the emitInsBinary branch May 30, 2018 04:14

		@@ -2920,174 +2920,283 @@ void emitter::emitInsStoreLcl(instruction ins, emitAttr attr, GenTreeLclVarCommo

		regNumber emitter::emitInsBinary(instruction ins, emitAttr attr, GenTree* dst, GenTree* src)

Conversation

tannergooding commented Jan 12, 2018

Uh oh!

tannergooding commented Jan 12, 2018

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!

mikedn Jan 18, 2018 • 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!

tannergooding Jan 12, 2018 • 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!

mikedn commented Jan 12, 2018

Uh oh!

tannergooding commented Jan 12, 2018

Uh oh!

CarolEidt left a comment

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!

CarolEidt commented Jan 17, 2018

Uh oh!

tannergooding commented Jan 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tannergooding Jan 18, 2018 • 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!

AndyAyersMS commented Jan 18, 2018

Uh oh!

tannergooding commented Jan 18, 2018

Uh oh!

AndyAyersMS commented Jan 18, 2018

Uh oh!

CarolEidt commented Jan 18, 2018

Uh oh!

tannergooding commented Jan 18, 2018

Uh oh!

BruceForstall commented Jan 18, 2018

Uh oh!

BruceForstall left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

mikedn Jan 18, 2018 •

edited

Loading

tannergooding Jan 12, 2018 •

edited

Loading

tannergooding Jan 18, 2018 •

edited

Loading

tannergooding commented Jan 19, 2018 •

edited

Loading