Refactor init/copy block codegen #27035

mikedn · 2019-10-04T17:08:17Z

Extracted from #21711 for easier handling.

A bunch of cleanup to make the init/copy block codegen code more consistent, readable and easier to change. One notable change is that GenTreeAddrMode is no longer a large node, it didn't need be. This simplifies things a bit because the original ADD node can be directly turned into a LEA node, instead of creating a new node and having to find the use and update it.

mikedn · 2019-10-04T18:15:27Z

src/jit/lower.cpp

-            // object). This is not a "regular" indirection.
-            // (Note that the user check could be costly.)
-            LIR::Use indirUse;
-            if (BlockRange().TryGetUse(indir, &indirUse) && indirUse.User()->OperIsIndir())


The removal of this code is responsible for the sole diff in this PR:

--- D:\Projects\jit\diffs\dasmset_16\base\System.Private.CoreLib.dasm 2019-10-04 20:51:01.000000000 +-0300 +++ D:\Projects\jit\diffs\dasmset_16\diff\System.Private.CoreLib.dasm 2019-10-04 20:51:01.000000000 +-0300 @@ -439051,6 +439051,5 @@ G_M6933_IG02: - add rdx, r8 - movups xmm0, xmmword ptr [rdx] + movups xmm0, xmmword ptr [rdx+r8] movups xmmword ptr [rcx], xmm0 mov rax, rcx

ContainCheckHWIntrinsicAddr calls TryCreateAddrMode with isIndir = true but then TryCreateAddrMode decides that it knows better...

mikedn · 2019-10-04T18:19:53Z

src/jit/emitxarch.cpp

-
-    dispIns(id);
-    emitCurIGsize += sz;
+    emitIns_R_ARX(ins, attr, reg, base, REG_NA, 1, disp);


A ton of duplicated code...

mikedn · 2019-10-04T18:24:27Z

src/jit/lowerarmarch.cpp

            }
-            blkNode->gtBlkOpKind = GenTreeBlk::BlkOpKindUnroll;
+
+            src->AsIntCon()->SetIconValue(fill);


The value wasn't updated when it was 0, even though the value that was checked for wasn't the original value but a masked one...

Nice catch, so if src->AsIntCon()->IconValue() == 0x11100 we would get a wrong init?
We should be able to repro it with CEE_INITBLK il.

Umm, I changed because it simply was confusing - lowering was masking and checking for 0 and codegen was NOT masking but also checking for 0. But yes, it turns out that it is a real bug on ARM:

52822000 movz w0, #0x1100 72A00020 movk w0, #1 LSL #16 9101C3A1 add x1, fp, #112 // [V05 loc0] B9000020 str w0, [x1] 39001020 strb w0, [x1,#4]

XARCH lowering was correct but also did things differently - masking, multiplying & updating, checking for 0.

mikedn · 2019-10-04T18:28:30Z

src/jit/lowerxarch.cpp

                {
-                    initVal->gtIntCon.gtIconVal = 0x01010101 * fill;
+                    fill *= 0x0101010101010101LL;
+                    src->gtType = TYP_LONG;


This changes the type of the constant but doesn't change the type of GT_INIT_VAL. Doesn't seem to have any ill side effects so I left it unchanged. Anyway I think that the type of GT_INIT_VAL should really be TYP_STRUCT.

mikedn · 2019-10-04T18:29:57Z

src/jit/lowerxarch.cpp

                {
-                    initVal->gtIntCon.gtIconVal = 0x01010101 * fill;
+                    fill *= 0x0101010101010101LL;
+                    src->gtType = TYP_LONG;


Also, the type was changed to long even when the fill was 0. That isn't necessary and resulted in xor rax, rax rather than xor eax, eax being generated in disassembly.

Did you see diffs from changing the handling of 0? Seems like that would be pretty common

There are only textual diffs. There are hacks in the emitter that prevented a 64 bit xor from being emitted in the first place, like here:

coreclr/src/jit/emitxarch.cpp

Lines 11360 to 11366 in 088299e

case EA_8BYTE:

// TODO-AMD64-CQ: Better way to not emit REX.W when we don't need it

// Don't need to zero out the high bits explicitly

if ((ins != INS_xor) || (reg1 != reg2))

{

code = AddRexWPrefix(ins, code);

}

mikedn · 2019-10-04T18:30:37Z

src/jit/lowerxarch.cpp

+                // so we need to check if there's a long enough sequence of non-GC slots.
+                ClassLayout* layout = blkNode->GetLayout();
+                unsigned     slots  = layout->GetSlotCount();
+                for (unsigned i = 0; i < slots; i++)


Why use 3 loops when you can do it with one?!

mikedn · 2019-10-04T18:31:16Z

src/jit/lowerxarch.cpp

-                addr->ClearContained();
-            }
-        }
-        else if (!source->IsMultiRegCall() && !source->OperIsSimdOrHWintrinsic())


The multireg call/simd/hw intrinsic does not ever happen.

BruceForstall · 2019-10-04T19:29:16Z

@dotnet/jit-contrib

sandreenko

Thanks for extracting this out of #21711 .
Some questions about the first 8 commits here, will return later.

I think you could speed up review/merge of these changes if cut them on even smaller parts (for example 7ed454a could be merged right now if you extract it into a separate PR, 9ab3434 also could be merged and it doesn't look like part of "Refactor init/copy block codegen").

src/jit/emitxarch.cpp

sandreenko · 2019-10-08T01:23:51Z

src/jit/codegenarmarch.cpp


-    assert(dstAddr->isUsedFromReg());


why have you decided to delete these asserts?

genConsumeReg eventually asserts if there's no register assigned to the node

coreclr/src/jit/codegenlinear.cpp

Line 1306 in 4e702da

assert(tree->gtHasReg());

So the assert is redundant. In general the JIT codebase seems to be having a problem with assert redundancy.

src/jit/codegenarmarch.cpp

src/jit/lowerxarch.cpp

src/jit/lsraarmarch.cpp

sandreenko · 2019-10-08T02:05:06Z

src/jit/lsraxarch.cpp

                        }
 #endif
                        buildInternalIntRegisterDefForNode(blkNode, regMask);
                    }

                    if (size >= XMM_REGSIZE_BYTES)
                    {
-                        // If we have a buffer larger than XMM_REGSIZE_BYTES,


Were these comments wrong or unnecessary?

The commenting throughout the JIT codebase is in general pretty terrible. The first rule about commenting is that comments should explain why the code does something when that isn't obvious and NOT what the code does. What the code does should be obvious from, well, the code itself.

Now, in complex code it's not always possible to write clear enough code. Or sometimes the code is clear but is large and saying a few words about what it does is helpful because it speeds up understanding of the code.

That's not the case here. It's 3 lines of trivial code and 5 lines of comments.

mikedn · 2019-10-08T06:14:37Z

I think you could speed up review/merge of these changes if cut them on even smaller parts

Sorry about this monster PR. Initially I was just looking to get the destination address of init block contained because that was already done for copy block and was missing from init block for no obvious reason.

Unfortunately lowering, lsra build & codegen are rather tighly coupled so I had to investigate what needs changing and then things spiralled out of control. Unused variables, confusing names, duplicated code, code inconsistencies for now reason and whatnot.

mikedn · 2019-10-08T07:24:48Z

I think you could speed up review/merge of these changes if cut them on even smaller parts

BTW - it's probably not too late to do that. There are some 3 commits near the end that would definitely fit well in 2 separate PRs. Preferences?

sandreenko · 2019-10-08T07:26:40Z

BTW - it's probably not too late to do that. There are some 3 commits near the end that would definitely fit well in 2 separate PRs. Preferences?

It would be nice, in additional I think it is useful to get independent ci testing for each of them to make sure that they don't hide regressions from each others.

sandreenko

a few more questions/comments.

sandreenko · 2019-10-08T19:12:49Z

src/jit/lowerarmarch.cpp

            }
-            blkNode->gtBlkOpKind = GenTreeBlk::BlkOpKindUnroll;
+
+            src->AsIntCon()->SetIconValue(fill);


Nice catch, so if src->AsIntCon()->IconValue() == 0x11100 we would get a wrong init?
We should be able to repro it with CEE_INITBLK il.

sandreenko · 2019-10-08T19:21:22Z

src/jit/lowerarmarch.cpp

-
-#ifdef DEBUG
-            // CpObj must always have at least one GC-Pointer as a member.
-            assert(objNode->GetLayout()->HasGCPtr());


Hm, before we were checking:

if (blkNode->OperGet() == GT_STORE_OBJ) assert(blkNode->AsObj()->GetLayout()->HasGCPtr());

now there is

if (blkNode->OperIs(GT_STORE_OBJ) && (!blkNode->AsObj()->GetLayout()->HasGCPtr() || blkNode->gtBlkOpGcUnsafe))

so if before all GT_STORE_OBJ had at least one GCPtr then who passes !blkNode->AsObj()->GetLayout()->HasGCPtr() check after that change?

Ah, nice catch. This change shouldn't probably be in this PR, though it's small and doesn't hurt.

One of the removed commits (mikedn@8336b44) is moving the code from fgMorphUnsafeBlk to lowering. That's responsible for changing OBJs without GC pointers into BLKs.

I'm also working on #27053 that will make BLKs even more rare. Since introducing ClassLayout and making OBJ a small node there's no longer any need to discard struct type information from IR. We should use OBJ whenever type information is available, with or without GC pointers.

I believe it's still the case that after morph we have no GT_OBJ nodes that have zero gc pointers. So I'm not sure why this would change.

Yes, currently GT_OBJ nodes always have GC pointers. The plan is to use GT_OBJ nodes even in cases when no GC pointers are present, at least until lowering. After lowering it probably doesn't matter anymore.

Makes sense - in that case perhaps we don't need to change it.

src/jit/lowerarmarch.cpp

sandreenko · 2019-10-08T21:55:29Z

src/jit/codegenarmarch.cpp

+//   b) The size of the struct to initialize is smaller than INITBLK_UNROLL_LIMIT bytes.
+void CodeGen::genCodeForInitBlkUnroll(GenTreeBlk* initBlkNode)
+{
+#ifndef _TARGET_ARM64_


why not #ifdef _TARGET_ARM_?
are you going to implement this function later?

Yes, the ifdef will just disappear in #21711. Besides, I find it convenient to place the small block before the big block when you some sort of if/else construct. You don't need to scroll through the entire big block to see what the small block is doing.

CarolEidt

A couple of questions and some minor comment nits.

CarolEidt · 2019-10-09T01:14:33Z

src/jit/codegenarmarch.cpp

+    {
+        dstAddrBaseReg = genConsumeReg(dstAddr);
+
+        // TODO-Cleanup: This matches the old code behavior in order to get 0 diffs. It's otherwise


Is this something you're planning to change? Should there be an issue?
Also, it should be TODO-ARMArch.

Yes, this will be fixed in the PR from where all this cleanup was extracted from: 1004804

src/jit/codegenxarch.cpp

CarolEidt · 2019-10-09T18:55:09Z

src/jit/lowerarmarch.cpp

-
-#ifdef DEBUG
-            // CpObj must always have at least one GC-Pointer as a member.
-            assert(objNode->GetLayout()->HasGCPtr());


I believe it's still the case that after morph we have no GT_OBJ nodes that have zero gc pointers. So I'm not sure why this would change.

src/jit/lowerarmarch.cpp

CarolEidt · 2019-10-09T19:04:05Z

src/jit/lowerarmarch.cpp


-            size_t classSize = objNode->GetLayout()->GetSize();
-            size_t blkSize   = roundUp(classSize, TARGET_POINTER_SIZE);
+        if (blkNode->OperIs(GT_STORE_OBJ) && (!blkNode->AsObj()->GetLayout()->HasGCPtr() || blkNode->gtBlkOpGcUnsafe))


I see that this check was here before. It should probably be asserting !blkNode->AsObj()->GetLayout()->HasGCPtr(), but perhaps that's best left for another PR.

CarolEidt · 2019-10-09T19:06:01Z

src/jit/lowerxarch.cpp

            {
+                // TODO-CQ: We could unroll even when the initialization value is not a constant


Nit: TODO-XArch-CQ

While this is in lowerxarch it is not arch specific, you can always insert a mul x, 0x01010101 to deal with non-const initialization. Not sure if anyone is doing this, though since initblk was exposed in the Unsafe class perhaps someone will complain about it.

CarolEidt · 2019-10-09T19:07:04Z

src/jit/lowerxarch.cpp

                {
-                    initVal->gtIntCon.gtIconVal = 0x01010101 * fill;
+                    fill *= 0x0101010101010101LL;
+                    src->gtType = TYP_LONG;


Did you see diffs from changing the handling of 0? Seems like that would be pretty common

mikedn · 2019-10-10T16:22:38Z

I haven't rebased this yet in case there is more feedback.

CarolEidt

LGTM - thanks!

sandreenko · 2019-10-10T17:55:51Z

/azp run outerloop

azure-pipelines · 2019-10-10T17:55:55Z

Pull request contains merge conflicts.

sandreenko · 2019-10-10T17:57:25Z

Unfortunatle I won't be able to do a review until Monday, could you please do a rebase now so we can check pri1 and stress jobs?

mikedn · 2019-10-10T18:16:01Z

Thanks, conflicts fixed. Just gtRegNum/GetRegNum stuff.

sandreenko · 2019-10-10T18:36:15Z

/azp run outerloop

azure-pipelines · 2019-10-10T18:36:20Z

No pipelines are associated with this pull request.

mikedn · 2019-10-10T20:35:39Z

Huh, the assert I added to check for address containment fired. I'd say that the assert is in itself correct, what's dubious is the IR that gets produced from this piece of IL:

    [ 0]   0 (0x000) ldloca.s 0
    [ 1]   2 (0x002) ldarg.0
    [ 2]   3 (0x003) ldflda 04000002
    [ 2]   8 (0x008) ldc.i4.s 28
    [ 3]  10 (0x00a) cpblk

is imported as:

               [000007] -A-XG-------              *  ASG       struct (copy)
               [000006] -------N----              +--*  BLK(28)   struct
               [000001] ------------              |  \--*  ADDR      byref 
               [000000] ------------              |     \--*  LCL_VAR   struct V02 loc0         
               [000003] ---XG--N----              \--*  FIELD     int    f2
               [000002] ------------                 \--*  LCL_VAR   byref  V01 arg0

which eventually ends up as

N001 (  1,  1) [000002] ------------         t2 =    LCL_VAR   byref  V01 arg0         u:1 (last use) $81
N002 (  1,  1) [000014] ------------        t14 =    CNS_INT   long   4 field offset Fseq[f2] $100
                                                  /--*  t2     byref  
                                                  +--*  t14    long   
N003 (  2,  2) [000015] -------N----        t15 = *  ADD       byref  $140
                                                  /--*  t15    byref  
N004 (  4,  4) [000003] *--XG--N----         t3 = *  IND       int    <l:$241, c:$240>
N005 (  3,  2) [000000] U------N----         t0 =    LCL_VAR_ADDR byref  V02 loc0         ud:1->2
                                                  /--*  t0     byref  
                                                  +--*  t3     int    
N007 (  6,  5) [000006] nA-XG-------              *  STORE_BLK(28) struct (copy)

And the indir address gets contained because its type is TYP_INT. IMO we should not end up with such a struct/block-op/primitive mix. The importer (and possibly block copy morph) should be fixed.

Interestingly, I've run into a somewhat related issue with initobj recently - that gets blindly imported as BLK, even if the class handle is a primitive or reference type. And attempting to generate an OBJ instead of BLK like I'm trying to do in another PR results in the creation of a ClassLayout object for a ref type. Fun.

mikedn · 2019-10-11T07:46:04Z

However, I still think that at least the first 4 commits are independent enough to be merged separately in 4 independent PRs

Sure, I've no problem splitting this even further. The main reason the original PR got so big is that for a long time nothing was merged and it would have been difficult for me to keep multiple PRs coordinated. Not to mention that the CI just isn't reliable, more builds -> more chance of random failures.

sandreenko · 2019-10-11T08:36:02Z

Sure, I've no problem splitting this even further. The main reason the original PR got so big is that for a long time nothing was merged and it would have been difficult for me to keep multiple PRs coordinated. Not to mention that the CI just isn't reliable, more builds -> more chance of random failures.

That is true, sorry about that, but we are trying to do these things better now...

Normally 0 sized block ops should be simply removed during lowering. But these are rare enough that adding special code to deal with them is questionable. Instead unroll such block ops, the unroll codegen simply won't generate any code for a 0 sized block op. Of course, some registers will still be allocated.

Non-GC GT_STORE_OBJ nodes were changed to GT_STORE_BLK in block copy ops but not in block init ops. Since block init is effectvely non-GC it is preferable to be consistent and change to GT_STORE_BLK in both cases.

sandreenko · 2019-10-15T19:29:56Z

/azp list

azure-pipelines · 2019-10-15T19:30:01Z

CI/CD Pipelines for this repository: coreclr-outerloop coreclr-ci dotnet-coreclr-nullablefeature coreclr-r2r coreclr-gc-longrunning

sandreenko · 2019-10-15T19:30:35Z

/azp run coreclr-outerloop

azure-pipelines · 2019-10-15T19:30:49Z

Azure Pipelines successfully started running 1 pipeline(s).

CarolEidt

Just one question about removing a TODO comment

CarolEidt · 2019-10-15T21:14:42Z

src/jit/lsraarmarch.cpp


-        if (blkNode->gtBlkOpKind == GenTreeBlk::BlkOpKindUnroll)
-        {
-            // TODO-ARM-CQ: Currently we generate a helper call for every


Is there a reason you removed this TODO?

The NYI_ARM bellow should suffice. And anyway I'll enable unroll on ARM in the "improve" PR: d871e37

OK, I can live with that, though believe it or not, I do occasionally grep for the TODOs.

We have that too but in lowering:

coreclr/src/jit/lowerarmarch.cpp

Lines 280 to 285 in 2424f34

#endif // _TARGET_ARM64_

{

// TODO-ARM-CQ: Currently we generate a helper call for every initblk we encounter.

// Later on we should implement loop unrolling code sequences to improve CQ.

blkNode->gtBlkOpKind = GenTreeBlk::BlkOpKindHelper;

}

sandreenko

LGTM, thanks again for splitting that.

Note: the stress testing looks greener than I expected it (no infra/timeouts)

sandreenko · 2019-10-15T23:00:44Z

Is it worth recollecting diffs now?

mikedn · 2019-10-16T17:48:45Z

I've ran x64/x86 and altjit arm32/arm64 diffs again - none. The one I mentioned in the first comment in this issue was coming from one of commits that was removed from this PR.

Looking through failed jitstress tests, looks like stuff with a history...

sandreenko · 2019-10-16T18:02:47Z

I've ran x64/x86 and altjit arm32/arm64 diffs again - none. The one I mentioned in the first comment in this issue was coming from one of commits that was removed from this PR.

I expected that Use GT_STORE_BLK/unroll consistently and Treat 0 sized block ops as unrolled commits could generate diffs. Doesn't it just happen in framework libraries or am I wrong that they could?

mikedn · 2019-10-16T18:15:25Z

Use GT_STORE_BLK/unroll consistently

Hmm, why? The inconsistency was that init block codegen accepted both BLK and OBJ and generated the same code in both cases while copy block also accepted both but generated different code. The change simply makes init block uses only BLK.

Treat 0 sized block ops as unrolled

Yes, that would generate diffs if you run into such a case. Let me look for a test that does this, I'm pretty sure there's at least one.

sandreenko · 2019-10-16T18:28:56Z

Hmm, why? The inconsistency was that init block codegen accepted both BLK and OBJ and generated the same code in both cases while copy block also accepted both but generated different code. The change simply makes init block uses only BLK.

Got it, I had the impression that we were calling emitDisableGC in init block for OBJ. Thanks.

mikedn · 2019-10-16T18:35:49Z

Here's a 0 sized block test that @CarolEidt recently added while fixing a bug: https://github.com/dotnet/coreclr/blob/master/tests/src/JIT/Regression/JitBlue/GitHub_24846/GitHub_24846.cs

TestCopy produces the best diff:

--- D:\Projects\jit\diffs\dasmset_1\base\test.dasm	2019-10-16 21:18:05.000000000 +-0300
+++ D:\Projects\jit\diffs\dasmset_1\diff\test.dasm	2019-10-16 21:18:05.000000000 +-0300
@@ -15,14 +15,4 @@
 G_M38074_IG01:
-       push     rdi
-       push     rsi
-       mov      rdi, rcx
 
 G_M38074_IG02:
-       mov      rsi, rdx
-       xor      ecx, ecx
-       rep movsb 
 
-G_M38074_IG03:
-       pop      rsi
-       pop      rdi
        ret

Since unrolled block copy does not place any restrictions on the source and destination registers the JIT doesn't even need to move the registers so no code is being generated.

The block init case is more interesting - you can see how registers are still getting allocated even if no actual initialization code is generated:

--- D:\Projects\jit\diffs\dasmset_1\base\test.dasm	2019-10-16 21:18:05.000000000 +-0300
+++ D:\Projects\jit\diffs\dasmset_1\diff\test.dasm	2019-10-16 21:18:05.000000000 +-0300
@@ -160,6 +145,4 @@
 G_M7755_IG05:
-       xor      rsi, rsi
-       mov      rdi, qword ptr [rbp-20H]
-       xor      ecx, ecx
-       rep movsb 
+       xor      rax, rax
+       mov      rdx, qword ptr [rbp-18H]

At least the xor could probably be removed by making the initialization value contained - since no initialization is done a register is not needed. But that's just more code to handle a very rare case.

mikedn · 2019-10-16T18:38:41Z

Got it, I had the impression that we were calling emitDisableGC in init block for OBJ. Thanks.

Something like that actually happens but it's a different issue and the fix is in one of the removed commits. Anyway, it doesn't result in actual code differences, you'll only see less labels in diffs due to the GC non-interruptible IGs created by the emitter for disable/enable GC.

mikedn force-pushed the blk-cleanup branch from f0eff3f to e63c8f1 Compare October 4, 2019 17:54

mikedn commented Oct 4, 2019

View reviewed changes

BruceForstall requested a review from sandreenko October 4, 2019 19:29

BruceForstall added the area-CodeGen label Oct 4, 2019

mikedn force-pushed the blk-cleanup branch 2 times, most recently from e104e49 to 8336b44 Compare October 7, 2019 05:08

sandreenko reviewed Oct 8, 2019

View reviewed changes

mikedn force-pushed the blk-cleanup branch from 8336b44 to 98278c6 Compare October 8, 2019 15:17

sandreenko reviewed Oct 8, 2019

View reviewed changes

CarolEidt reviewed Oct 9, 2019

View reviewed changes

CarolEidt approved these changes Oct 10, 2019

View reviewed changes

mikedn force-pushed the blk-cleanup branch from f40fdda to 604f31b Compare October 10, 2019 18:11

This was referenced Oct 11, 2019

Remove duplicate code from emitIns_R_AR[R|X]/AR[R|X]_R #27145

Merged

Cleanup unrolled block op codegen #27146

Merged

mikedn force-pushed the blk-cleanup branch from c81a5af to c8b23d4 Compare October 12, 2019 05:45

mikedn mentioned this pull request Oct 13, 2019

Cleanup LowerBlockStore #27170

Merged

mikedn added 5 commits October 15, 2019 19:49

Delete INITBLK/CPBLK_STOS_LIMIT

e8cc256

Cleanup BuildBlockStore

ac74cc7

BlkOpKindHelper is not available on x86

4c03e79

Use GT_STORE_BLK/unroll consistently

2424f34

Non-GC GT_STORE_OBJ nodes were changed to GT_STORE_BLK in block copy ops but not in block init ops. Since block init is effectvely non-GC it is preferable to be consistent and change to GT_STORE_BLK in both cases.

mikedn force-pushed the blk-cleanup branch from c8b23d4 to 2424f34 Compare October 15, 2019 17:02

CarolEidt reviewed Oct 15, 2019

View reviewed changes

sandreenko approved these changes Oct 15, 2019

View reviewed changes

sandreenko merged commit 8370e67 into dotnet:master Oct 16, 2019

mikedn deleted the blk-cleanup branch October 16, 2019 18:46

mikedn mentioned this pull request Oct 18, 2019

Cleanup Lowering::TryCreateAddrMode #27284

Merged

This was referenced Jan 31, 2020

Do not create unnecessary LEA(b+0) nodes dotnet/runtime#13548

Closed

initblk incorrectly handles 0 fill on ARM64 dotnet/runtime#13579

Closed

	case EA_8BYTE:
	// TODO-AMD64-CQ: Better way to not emit REX.W when we don't need it
	// Don't need to zero out the high bits explicitly
	if ((ins != INS_xor) \|\| (reg1 != reg2))
	{
	code = AddRexWPrefix(ins, code);
	}

		{
		// TODO-CQ: We could unroll even when the initialization value is not a constant

	#endif // _TARGET_ARM64_
	{
	// TODO-ARM-CQ: Currently we generate a helper call for every initblk we encounter.
	// Later on we should implement loop unrolling code sequences to improve CQ.
	blkNode->gtBlkOpKind = GenTreeBlk::BlkOpKindHelper;
	}

Refactor init/copy block codegen #27035

Refactor init/copy block codegen #27035

Conversation

mikedn commented Oct 4, 2019 • edited Loading

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

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

BruceForstall commented Oct 4, 2019

sandreenko left a 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

mikedn commented Oct 8, 2019

mikedn commented Oct 8, 2019

sandreenko commented Oct 8, 2019

sandreenko left a 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

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

CarolEidt left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikedn commented Oct 10, 2019

CarolEidt left a comment

Choose a reason for hiding this comment

sandreenko commented Oct 10, 2019

azure-pipelines bot commented Oct 10, 2019

sandreenko commented Oct 10, 2019

mikedn commented Oct 10, 2019

sandreenko commented Oct 10, 2019

azure-pipelines bot commented Oct 10, 2019

mikedn commented Oct 10, 2019 • edited Loading

mikedn commented Oct 11, 2019

sandreenko commented Oct 11, 2019

sandreenko commented Oct 15, 2019

azure-pipelines bot commented Oct 15, 2019

sandreenko commented Oct 15, 2019

azure-pipelines bot commented Oct 15, 2019

CarolEidt left a 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

sandreenko left a comment

Choose a reason for hiding this comment

sandreenko commented Oct 15, 2019

mikedn commented Oct 16, 2019

sandreenko commented Oct 16, 2019

mikedn commented Oct 16, 2019

sandreenko commented Oct 16, 2019

mikedn commented Oct 16, 2019

mikedn commented Oct 16, 2019

mikedn commented Oct 4, 2019 •

edited

Loading

mikedn commented Oct 10, 2019 •

edited

Loading