Block the hoisting of TYP_STRUCT rvalues in loop hoisting by briansull · Pull Request #23739 · dotnet/coreclr

briansull · 2019-04-04T20:43:52Z

PR #22255 introduced test regressions:

Assertion failed 'tree->gtHasReg()' in 'Program:Test(ref):this' (IL size 80)

mikedn · 2019-04-05T18:28:46Z

+        structPassingKind howToPassStruct;
+        var_types         structBaseType = getArgTypeForStruct(structHnd, &howToPassStruct, false, size);
+
+        // Only allow structs whose storage can be held in a register to be CSE candidates.


Presumably that's just because it's not very useful to CSE something that doesn't fit in a register. Or is there a correctness issue as well?

I suppose it would make sense to CSE a struct indir if the new temporary variable can then be promoted. But that's probably quite complicated to get right.

Yes, There currently is a correctness issue as well when we perform loop hoisting we place a l-value which is a load (a GT_IND of a TYP_STRUCT ) into the loop pre-header.
This indirection doesn't get handled properly (or removed) and we end up asserting about a missing register during codegen.

see bool Compiler::optHoistLoopExprsForTree()
where we currently require:

// Tree must be a suitable CSE candidate for us to be able to hoist it. treeIsHoistable &= optIsCSEcandidate(tree);

Ah, of course, loop hoisting will hoist the LHS of the struct assignment but that does not have a struct handle so there's no way for it to actually work.

Still not clear to me if the above is a legality check followed by a profitability check, or if both are legality checks. Can you clarify?

@briansull - in our discussions of this, it seemed like the fundamental issues are:

The struct handling in the JIT relies on the fact that a struct r-value will always be used in a context where its size and/or handle are carried by an l-value (e.g. a block node on the lhs of an assign, or a block store).

An unused hoisted rvalue doesn't get removed. This leaves the problematic handle-less size-less struct indirection.

Before taking this fix, I'd like us to understand what diffs this creates, and file an issue to address the above problems.

As we improve our struct codegen, I'd expect us to see more cases where hoisting struct values would be beneficial, as we can't (and probably never will) promote all structs that might be created redundantly (within a loop or otherwise).

An unused hoisted rvalue doesn't get removed. This leaves the problematic handle-less size-less struct indirection.

Not only that it doesn't get removed but it simply cannot be used at all. Because loop hoisting is expecting CSE to assign the hoisted tree to a variable but CSE can't do that because there's struct type information available in the r-value.

To make this work with the current loop hoisiting implementation we'd need to stop discarding struct information from r-values (converting OBJ to TYP_STRUCT IND). That's doable and IMO desirable, discarding struct information is just risky.

FWIW I have seen many examples where manual struct CSEs are profitable -- for example the before version of this did not cache Start and End as local structs. IL semantics for structs tends to lead to lots of struct copies and often many of those copies are identical.

coreclr/src/System.Private.CoreLib/shared/System/Range.cs

Lines 105 to 122 in 0d581c7

[MethodImpl(MethodImplOptions.AggressiveInlining)]

public OffsetAndLength GetOffsetAndLength(int length)

{

int start;

Index startIndex = Start;

if (startIndex.IsFromEnd)

start = length - startIndex.Value;

else

start = startIndex.Value;

int end;

Index endIndex = End;

if (endIndex.IsFromEnd)

end = length - endIndex.Value;

else

end = endIndex.Value;

if ((uint)end > (uint)length || (uint)start > (uint)end)

Hmm, should perhaps this fix be restricted to loop hoisting?

I have coded up an alternative fix that blocks the hoisting of TYP_STRUCT r-values.

tannergooding · 2019-04-05T18:35:54Z

+        structPassingKind howToPassStruct;
+        var_types         structBaseType = getArgTypeForStruct(structHnd, &howToPassStruct, false, size);
+
+        // Only allow structs whose storage can be held in a register to be CSE candidates.


Is "register" referring just to integer registers and to the lower element of vector registers (for float/double)?

I imagine we may want to support CSE (and other optimizations) for full vector registers eventually...

The call to getArgTypeForStruct should identify the register sized SIMD struct types as
(howToPassStruct == SPK_PrimitiveType)

Currently it doesn't appear that we support passing such vector types in registers according to getArgTypeForStruct and getPrimitiveTypeForStruct

I believe @CarolEidt is working on this for the Vector ABI support.

sandreenko · 2019-04-05T19:19:59Z

cc @dotnet/jit-contrib.

@briansull is it possible to create a CoreClr repro based on desktop failures for this issue?

briansull · 2019-04-05T20:13:08Z

I will investigate creating a CoreCLR repro test case

mikedn · 2019-04-05T21:23:05Z

Here's a repro in case you don't have one already:

    struct Str
    {
        float a, b, c, d, e, f;
    }

    class Cls
    {
        public Str s;
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    static void Sink<T>(ref T t)
    {
    }

    static void Main() => new Program().Test(new Cls());

    [MethodImpl(MethodImplOptions.NoInlining)]
    void Test(Cls c)
    {
        Str l1 = default;
        Str l2 = default;
        Str l3 = default;

        for (int i = 0; i < 10; i++)
        {
            l1 = c.s;
            l2 = c.s;
            l3 = c.s;
        }

        Sink(ref l1);
        Sink(ref l2);
        Sink(ref l3);
    }

asserts:

Assert failure(PID 14128 [0x00003730], Thread: 16408 [0x4018]): Assertion failed 'tree->gtHasReg()' in 'Program:Test(ref):this' (IL size 80)

    File: d:\projects\coreclr\src\jit\codegenlinear.cpp Line: 1289
    Image: D:\Projects\coreclr\bin\Product\Windows_NT.x64.Checked\CoreRun.exe

briansull · 2019-04-05T21:49:27Z

Thank you @mikedn

briansull · 2019-04-06T01:36:33Z

Test case added

briansull · 2019-04-06T01:37:02Z

PTAL @dotnet/jit-contrib.

AndyAyersMS

Does this cause any diffs?

AndyAyersMS · 2019-04-08T16:01:18Z

+        structPassingKind howToPassStruct;
+        var_types         structBaseType = getArgTypeForStruct(structHnd, &howToPassStruct, false, size);
+
+        // Only allow structs whose storage can be held in a register to be CSE candidates.


Still not clear to me if the above is a legality check followed by a profitability check, or if both are legality checks. Can you clarify?

briansull · 2019-04-08T16:54:50Z

@dotnet-bot Test Windows_NT arm Cross Checked Innerloop Build and Test

briansull · 2019-04-08T17:08:53Z

On the desktop there are no reported Asm Diffs in the frameworks.

briansull · 2019-04-08T20:51:13Z

I have coded up an alternative fix that blocks the hoisting of TYP_STRUCT r-values.
PTAL @CarolEidt @AndyAyersMS @dotnet/jit-contrib.

Working on diffs

CarolEidt

LGTM - with one comment suggestion

CarolEidt · 2019-04-08T21:13:57Z

        {
-            GenTreeCall* call = tree->AsCall();
-            if (call->gtCallType != CT_HELPER)
+            // We cannot hoist an r-value of TYP_STRUCT, as it is illegal to do so


I would reword this comment as "We cannot hoist an r-value of TYP_STRUCT, as they currently do not carry full descriptors of the struct type" or something to that effect.

Added test case GitHub_23739.cs

briansull · 2019-04-08T22:23:43Z

Rebased and squashed commits

No Asm diffs

Block the hoisting of TYP_STRUCT rvalues in loop hoisting Commit migrated from dotnet/coreclr@cd41863

briansull changed the title ~~Only allow structs whose storage can be held in a register to be CS candidates~~ Only allow structs whose storage can be held in a register to be CSE candidates Apr 5, 2019

briansull force-pushed the struct-cse branch from f92b1bf to 921d502 Compare April 5, 2019 16:46

mikedn reviewed Apr 5, 2019

View reviewed changes

tannergooding reviewed Apr 5, 2019

View reviewed changes

AndyAyersMS reviewed Apr 8, 2019

View reviewed changes

briansull changed the title ~~Only allow structs whose storage can be held in a register to be CSE candidates~~ Block the hoisting of TYP_STRUCT rvalues in loop hoisting Apr 8, 2019

CarolEidt approved these changes Apr 8, 2019

View reviewed changes

Block the hoisting of TYP_STRUCT rvalues in loop hoisting

b87e634

Added test case GitHub_23739.cs

briansull force-pushed the struct-cse branch from 86d1c3c to b87e634 Compare April 8, 2019 22:23

briansull merged commit cd41863 into dotnet:master Apr 9, 2019

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Merge pull request dotnet/coreclr#23739 from briansull/struct-cse

e8cde25

Block the hoisting of TYP_STRUCT rvalues in loop hoisting Commit migrated from dotnet/coreclr@cd41863

	[MethodImpl(MethodImplOptions.AggressiveInlining)]
	public OffsetAndLength GetOffsetAndLength(int length)
	{
	int start;
	Index startIndex = Start;
	if (startIndex.IsFromEnd)
	start = length - startIndex.Value;
	else
	start = startIndex.Value;

	int end;
	Index endIndex = End;
	if (endIndex.IsFromEnd)
	end = length - endIndex.Value;
	else
	end = endIndex.Value;

	if ((uint)end > (uint)length \|\| (uint)start > (uint)end)

Uh oh!

Conversation

briansull commented Apr 4, 2019 • edited 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!

briansull Apr 5, 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!

briansull Apr 5, 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!

briansull Apr 5, 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!

sandreenko commented Apr 5, 2019

Uh oh!

briansull commented Apr 5, 2019

Uh oh!

mikedn commented Apr 5, 2019

Uh oh!

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

Uh oh!

Uh oh!

briansull commented Apr 6, 2019

Uh oh!

briansull commented Apr 6, 2019

Uh oh!

AndyAyersMS left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

briansull commented Apr 8, 2019

Uh oh!

briansull commented Apr 8, 2019

Uh oh!

briansull commented Apr 8, 2019

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!

briansull commented Apr 8, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

briansull commented Apr 4, 2019 •

edited

Loading

briansull Apr 5, 2019 •

edited

Loading

briansull Apr 5, 2019 •

edited

Loading

briansull Apr 5, 2019 •

edited

Loading

briansull commented Apr 5, 2019 •

edited

Loading