Improvements to loop hoisting #71504

BruceForstall · 2022-06-30T20:17:25Z

optHoistLoopNest(): (1) no need to check if a pre-header has already been added;
pre-headers are unique to a loop if they exist. (2) simplify the "append to list" code.
optHoistThisLoop(): (1) allow adding multiple pre-headers after every "IDom" addition,
if there are any in the right order. (2) Simplify addition of remaining pre-headers.
optLoopEntry(): technically, you shouldn't look at bbJumpDest
unless you know the branch kind is one that sets it. So change the condition.

Also, fix the output of why something wasn't hoisted. The concept of "hoistable" implies
"invariant" (but not the reverse). So print the least specific failure reason. These were
backwards before. For instance, if we couldn't hoist a top node, but it was invariant, we
would, oddly, print "not invariant" instead of "not hoistable". In the case where it isn't
hoistable, and also isn't invariant, it makes more sense to print "not invariant", which
also implies "not hoistable".

Add dumping the OpName for the Pre/PostOrderVisit output, to help avoid cross-referencing
the node number all the time.

No asm diffs.

- optHoistLoopNest(): (1) no need to check if a pre-header has already been added; pre-headers are unique to a loop if they exist. (2) simplify the "append to list" code. - optHoistThisLoop(): (1) allow adding multiple pre-headers after every "IDom" addition, if there are any in the right order. (2) Simplify addition of remaining pre-headers. - optLoopEntry(): technically, you shouldn't look at `bbJumpDest` unless you know the branch kind is one that sets it. So change the condition. Also, fix the output of why something wasn't hoisted. The concept of "hoistable" implies "invariant" (but not the reverse). So print the least specific failure reason. These were backwards before. For instance, if we couldn't hoist a top node, but it was invariant, we would, oddly, print "not invariant" instead of "not hoistable". In the case where it isn't hoistable, and also isn't invariant, it makes more sense to print "not invariant", which also implies "not hoistable". Add dumping the OpName for the Pre/PostOrderVisit output, to help avoid cross-referencing the node number all the time. No asm diffs.

BruceForstall · 2022-06-30T20:20:22Z

While the change (1) to optHoistThisLoop() to allow adding multiple pre-headers after every "IDom" addition doesn't lead to any spmi asm diffs, it does lead to a different "Considering hoisting in blocks that either dominate exit block BB09 or preheaders of nested loops, if any" order in the following test case:

using System;
using System.Runtime.CompilerServices;
namespace ns
{
    class Program
    {
        public static int x, y;

        [MethodImpl(MethodImplOptions.NoInlining)]
        public static int foo(int i)
        {
            int result = 0;
            for (int j = 0; j < 100; j++)
            {
                result += (x * y);

                if (i > j)
                {
                    result += (x * y);
                }

                result += (x * y);

                if (i == j)
                {
                    for (int k = 0; k < 100; k++)
                    {
                        result += (x * y);
                    }

                    for (int k = 0; k < 100; k++)
                    {
                        result += (x * y);
                    }
                }
            }
            return result;
        }

	    public static void Main()
        {
            x = 1;
            y = 2;
            Console.WriteLine("{0}", foo(7));
        }
    }
}

BruceForstall · 2022-06-30T20:20:50Z

@kunalspathak @dotnet/jit-contrib PTAL

ghost · 2022-06-30T20:23:04Z

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

Issue Details

optHoistLoopNest(): (1) no need to check if a pre-header has already been added;
pre-headers are unique to a loop if they exist. (2) simplify the "append to list" code.
optHoistThisLoop(): (1) allow adding multiple pre-headers after every "IDom" addition,
if there are any in the right order. (2) Simplify addition of remaining pre-headers.
optLoopEntry(): technically, you shouldn't look at bbJumpDest
unless you know the branch kind is one that sets it. So change the condition.

Also, fix the output of why something wasn't hoisted. The concept of "hoistable" implies
"invariant" (but not the reverse). So print the least specific failure reason. These were
backwards before. For instance, if we couldn't hoist a top node, but it was invariant, we
would, oddly, print "not invariant" instead of "not hoistable". In the case where it isn't
hoistable, and also isn't invariant, it makes more sense to print "not invariant", which
also implies "not hoistable".

Add dumping the OpName for the Pre/PostOrderVisit output, to help avoid cross-referencing
the node number all the time.

No asm diffs.

Author:	BruceForstall
Assignees:	-
Labels:	`area-CodeGen-coreclr`
Milestone:	-

BruceForstall · 2022-06-30T23:00:11Z

No diffs

kunalspathak · 2022-06-30T23:10:02Z

it does lead to a different order

Do you mind sharing before vs. after Jitdump?

kunalspathak

Thanks for the cleanup. Add few suggestions.

src/coreclr/jit/optimizer.cpp

kunalspathak · 2022-06-30T23:06:33Z

src/coreclr/jit/optimizer.cpp

        while (cur != nullptr && pLoopDsc->lpContains(cur) && cur != pLoopDsc->lpEntry)
        {
            JITDUMP("  --  " FMT_BB " (dominate exit block)\n", cur->bbNum);
            defExec.Push(cur);
            cur = cur->bbIDom;

-            if (preHeadersList != nullptr)
+            for (; preHeadersList != nullptr; preHeadersList = preHeadersList->next)


Did you find out an example where we didn't add a preheader in this loop?

Did you find out an example where we didn't add a preheader in this loop?

Do you mean where we added more than one preheader? Zero or one would happen the same as before. (And the answer is yes)

Ok and the example is #71504 (comment)?

src/coreclr/jit/optimizer.cpp

BruceForstall · 2022-06-30T23:23:18Z

Do you mind sharing before vs. after Jitdump?

Here's the relevant fragment for the test case I included above:

Base:
--  BB09 (dominate exit block)
--  BB11 (preheader of L02)
--  BB04 (dominate exit block)
--  BB12 (preheader of L01)
--  BB02 (entry block)

Diff:
--  BB09 (dominate exit block)
--  BB11 (preheader of L02)
--  BB12 (preheader of L01)
--  BB04 (dominate exit block)
--  BB02 (entry block)

Note how BB04 and BB12 are swapped. In the "diff" case, we loop and add both BB11/BB12 after BB09.

kunalspathak · 2022-07-01T14:11:39Z

Note how BB04 and BB12 are swapped.

Can you paste the Block table just before we do the hoisting?

BruceForstall · 2022-07-01T16:24:14Z

Can you paste the Block table just before we do the hoisting?

-----------------------------------------------------------------------------------------------------------------------------------------
BBnum BBid ref try hnd preds           weight    lp [IL range]     [jump]      [EH region]         [flags]
-----------------------------------------------------------------------------------------------------------------------------------------
BB01 [0000]  1                             1       [000..006)-> BB10 ( cond )                     i
BB02 [0001]  2       BB01,BB09             4     0 [006..018)-> BB04 ( cond )                     i Loop Loop0 bwd bwd-target
BB03 [0002]  1       BB02                  2     0 [018..026)                                     i bwd
BB04 [0003]  2       BB02,BB03             4     0 [026..038)-> BB09 ( cond )                     i bwd
BB05 [0004]  1       BB04                  2     0 [038..03C)-> BB07 ( cond )                     i bwd
BB12 [0017]  1       BB05                  2     0 [03C..???)                                     internal LoopPH
BB06 [0005]  2       BB06,BB12            16     1 [03C..053)-> BB06 ( cond )                     i Loop Loop0 bwd bwd-target align
BB07 [0007]  2       BB05,BB06             2     0 [053..057)-> BB09 ( cond )                     i bwd
BB11 [0016]  1       BB07                  2     0 [057..???)                                     internal LoopPH
BB08 [0008]  2       BB08,BB11            16     2 [057..06E)-> BB08 ( cond )                     i Loop Loop0 bwd bwd-target align
BB09 [0010]  3       BB04,BB07,BB08        4     0 [06E..077)-> BB02 ( cond )                     i bwd
BB10 [0012]  2       BB01,BB09             1       [077..079)        (return)                     i
-----------------------------------------------------------------------------------------------------------------------------------------

***************  Natural loop table
L00, from BB02 to BB09 (Head=BB01, Entry=BB02, Exit=BB09), child loop = L02
L01, from BB06 to BB06 (Head=BB12, Entry=BB06, Exit=BB06, parent=L00) prehead
L02, from BB08 to BB08 (Head=BB11, Entry=BB08, Exit=BB08, parent=L00), sibling loop = L01 prehead

kunalspathak · 2022-07-01T16:26:43Z

Thanks. It is interesting to see that despite the reordering, we don't see any spmi-diff. How many methods have such cases?

BruceForstall · 2022-07-01T22:36:25Z

It is interesting to see that despite the reordering, we don't see any spmi-diff. How many methods have such cases?

Here are the 15 functions in SPMI collections that have 2 or more pre-headers to be added in the inner loop (that is, the reason why the inner loop should be a while instead of an if). Presumably even though they have blocks added in a different order, nothing got subsequently hoisted in a different order (or at all).

Benchstone.BenchF.Romber:Bench():bool
BitOps:DoBitfieldIteration(System.Int32[],System.Int32[],int,byref):long
DefaultBinder:BindToMethod(int,ref,byref,ref,CultureInfo,ref,byref):MethodBase:this
Microsoft.CodeAnalysis.CSharp.Symbols.ConstantEvaluationHelpers:OrderGraph(System.Collections.Generic.Dictionary`2[Microsoft.CodeAnalysis.CSharp.Symbols.SourceFieldSymbolWithSyntaxReference, Microsoft.CodeAnalysis.CSharp.Symbols.ConstantEvaluationHelpers+Node`1[Microsoft.CodeAnalysis.CSharp.Symbols.SourceFieldSymbolWithSyntaxReference]],Microsoft.CodeAnalysis.ArrayBuilder`1[Microsoft.CodeAnalysis.CSharp.Symbols.ConstantEvaluationHelpers+FieldInfo])
Microsoft.CodeAnalysis.CSharp.Symbols.OverriddenOrHiddenMembersHelpers:MakeInterfaceOverriddenOrHiddenMembers(Microsoft.CodeAnalysis.CSharp.Symbol,bool):Microsoft.CodeAnalysis.CSharp.Symbols.OverriddenOrHiddenMembersResult
Microsoft.CodeAnalysis.VisualBasic.Binder:ReportCommonErrorsFromLambdas(Microsoft.CodeAnalysis.ArrayBuilder`1[System.Collections.Generic.KeyValuePair`2[Microsoft.CodeAnalysis.VisualBasic.Symbol, System.Collections.Immutable.ImmutableArray`1[Microsoft.CodeAnalysis.Diagnostic]]],System.Collections.Immutable.ImmutableArray`1[Microsoft.CodeAnalysis.VisualBasic.BoundExpression],Microsoft.CodeAnalysis.DiagnosticBag):bool
Microsoft.CodeAnalysis.VisualBasic.Symbols.DeclarationTreeBuilder:GetNonTypeMemberNames(Microsoft.CodeAnalysis.SyntaxList`1[Microsoft.CodeAnalysis.VisualBasic.Syntax.StatementSyntax],byref):System.String[]:this
NuGet.Frameworks.FrameworkNameProvider:AddCompatibleCandidates():this
System.Data.XmlTreeGen:HandleTable(System.Data.DataTable,System.Xml.XmlDocument,System.Xml.XmlElement,bool):System.Xml.XmlElement:this
System.DirectoryServices.Protocols.DirectoryControl:TransformControls(System.DirectoryServices.Protocols.DirectoryControl[])
System.Globalization.CalendarData:NormalizeDatePattern(System.String):System.String
System.Globalization.DateTimeFormatInfoScanner:ScanDateWord(System.String):this
System.Net.Http.Http3RequestStream:BufferHeaderCollection(System.Net.Http.Headers.HttpHeaders):this
System.Security.Principal.WindowsIdentity:AddTokenClaims(System.Collections.Generic.List`1[System.Security.Claims.Claim],int,System.String):this
System.Xml.Schema.KeySequence:GetHashCode():int:this

kunalspathak

LGTM

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

BruceForstall requested a review from kunalspathak June 30, 2022 20:20

ghost assigned BruceForstall Jun 30, 2022

kunalspathak requested changes Jun 30, 2022

View reviewed changes

ghost added needs-author-action An issue or pull request that requires more info or actions from the author. and removed needs-author-action An issue or pull request that requires more info or actions from the author. labels Jun 30, 2022

Feedback

a0653d2

kunalspathak approved these changes Jul 1, 2022

View reviewed changes

BruceForstall merged commit f28904a into dotnet:main Jul 1, 2022

BruceForstall deleted the HoistImprovements branch July 1, 2022 23:45

JulieLeeMSFT mentioned this pull request Jul 28, 2022

What's new in .NET 7 Preview 7 [WIP] dotnet/core#7455

Closed

ghost locked as resolved and limited conversation to collaborators Aug 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improvements to loop hoisting #71504

Improvements to loop hoisting #71504

BruceForstall commented Jun 30, 2022

BruceForstall commented Jun 30, 2022

BruceForstall commented Jun 30, 2022

ghost commented Jun 30, 2022

BruceForstall commented Jun 30, 2022

kunalspathak commented Jun 30, 2022

kunalspathak left a comment

kunalspathak Jun 30, 2022

BruceForstall Jun 30, 2022

kunalspathak Jul 1, 2022

BruceForstall Jul 1, 2022

BruceForstall commented Jun 30, 2022

kunalspathak commented Jul 1, 2022

BruceForstall commented Jul 1, 2022

kunalspathak commented Jul 1, 2022

BruceForstall commented Jul 1, 2022

kunalspathak left a comment

Improvements to loop hoisting #71504

Improvements to loop hoisting #71504

Conversation

BruceForstall commented Jun 30, 2022

BruceForstall commented Jun 30, 2022

BruceForstall commented Jun 30, 2022

ghost commented Jun 30, 2022

BruceForstall commented Jun 30, 2022

kunalspathak commented Jun 30, 2022

kunalspathak left a comment

Choose a reason for hiding this comment

kunalspathak Jun 30, 2022

Choose a reason for hiding this comment

BruceForstall Jun 30, 2022

Choose a reason for hiding this comment

kunalspathak Jul 1, 2022

Choose a reason for hiding this comment

BruceForstall Jul 1, 2022

Choose a reason for hiding this comment

BruceForstall commented Jun 30, 2022

kunalspathak commented Jul 1, 2022

BruceForstall commented Jul 1, 2022

kunalspathak commented Jul 1, 2022

BruceForstall commented Jul 1, 2022

kunalspathak left a comment

Choose a reason for hiding this comment