Add JitOptRepeat debug config flags #7996

JosephTremoulet · 2016-11-04T14:53:26Z

Add flag JitOptRepeat that specifies a set of methods on which to
iteratively perform global optimizations multiple times, and flag
JitOptRepeatCount to set the number of iterations (default 2) to apply
when JitOptRepeat kicks in.

These flags are debug-only; they are intended to facilitate performing
experiments investigating optimization opportunities missed due to phase
ordering issues.

Also update fgResetForSsa (which is called between iterations) to clear heap data, defnums, and post-order numbers.

JosephTremoulet · 2016-11-04T14:54:11Z

@dotnet/jit-contrib PTAL. Verified no diffs (x64 Windows jit-diff) as-is, and passing DDR with repeat count hard-coded to 16.

BruceForstall · 2016-11-04T16:22:56Z

src/inc/clrconfigvalues.h

@@ -513,6 +513,8 @@ CONFIG_STRING_INFO_EX(INTERNAL_JitRange, W("JitRange"), "", CLRConfig::REGUTIL_d
 CONFIG_DWORD_INFO_EX(INTERNAL_JITRequired, W("JITRequired"), (unsigned)-1, "", CLRConfig::REGUTIL_default)
 CONFIG_DWORD_INFO_DIRECT_ACCESS(INTERNAL_JITRoundFloat, W("JITRoundFloat"), "")
 CONFIG_DWORD_INFO_EX(INTERNAL_JitSkipArrayBoundCheck, W("JitSkipArrayBoundCheck"), 0, "", CLRConfig::REGUTIL_default)
+CONFIG_STRING_INFO_EX(INTERNAL_JitOptRepeat, W("JitOptRepeat"), "Runs optimizer multiple times on the method", CLRConfig::REGUTIL_default)
+CONFIG_DWORD_INFO_EX(INTERNAL_JitOptRepeatCount, W("JitOptRepeatCount"), 2, "Number of times to repeat opts when repeating", CLRConfig::REGUTIL_default)


There should be no need to add these to clrconfigvalues.h as long as they are only used by RyuJIT, and not the VM or other JITs.

+1, Any changes to the jit interface will require more work and it is best to avoid them if possible.

Adding a config value does not constitute a JIT interface change. That said, @BruceForstall is correct: we should be able to confine these definitions to jitconfigvalues.h if they are only used by RyuJIT.

BruceForstall · 2016-11-04T16:26:19Z

src/jit/compiler.cpp


-        if (doEarlyProp)
+        do


Should this be a while loop so you can set iterations=0?

BruceForstall · 2016-11-04T16:28:09Z

src/jit/compiler.cpp

@@ -4359,6 +4359,7 @@ void Compiler::compCompile(void** methodCodePtr, ULONG* methodCodeSize, JitFlags
        bool doCopyProp      = true;
        bool doAssertionProp = true;
        bool doRangeAnalysis = true;
+        int  iterations      = 1;


shouldn't this be unsigned? JitOptRepeatCount can't be specified as negative.

The macros that define the config COMPLUS_ settings use signed int, I'm more inclined to match them than to make this unsigned. With the change to a while loop, specifying a negative repeat count would then be the same as specifying zero.

Ok. But specifying zero is bad currently -- same as specifying something very large due to "do" loop.

BruceForstall · 2016-11-04T16:29:27Z

src/jit/compiler.cpp

+//    in effect.
+
+void Compiler::ResetOptAnnotations()
+{


Can/should you assert that JitOptRepeat is set, and JitOptRepeatCount is >1?

BruceForstall · 2016-11-04T16:31:43Z

src/jit/compiler.cpp

+
+void Compiler::RecomputeLoopInfo()
+{
+    // Recompute reachability sets, dominators, and loops.


Same asserts here?

BruceForstall · 2016-11-04T16:40:09Z

src/jit/compiler.cpp


-        if (doSsa)
+        if (JitConfig.JitOptRepeat().contains(info.compMethodName, info.compClassName, &info.compMethodInfo->args))


almost all other uses of JitConfig.*.contains() are in compInitOptions(), so they will be ignored in the fallback JIT if altjit is set, and maybe also ignored in the inlinee compiler. You should probably do the same (and set a flag in Compiler on whether OptRepeat is on or not)

These need to be cleared for the same reasons as the locals' PerSsaData; their omission seems to be a simple oversight.

SSA construction will overwrite these annotations, but only for reachable code. Clear the annotations in all code in `fgResetForSsa` so that downstream analysis won't stumble over stale/invalid SSA annotations in unreachable code.

Add flag JitOptRepeat that specifies a set of methods on which to iteratively perform global optimizations multiple times, and flag JitOptRepeatCount to set the number of iterations (default 2) to apply when JitOptRepeat kicks in. These flags are debug-only; they are intended to facilitate performing experiments investigating optimization opportunities missed due to phase ordering issues.

The cache created in the first iteration may not be correct in the second. It seems there is a more general problem with failure to invalidate this cache (GitHub issue #7844), that this change is working around for the optRepeat case.

This works around an apparent bug in SSA construction (GitHub issue #7846), specifically `fgPerNodeLocalVarLiveness`, where heap uses are not considered upwards-exposed if they follow a heap def in their block (which is incorrect because the store and load are not necessarily must-alias). In the non-repeat configuration, these flags are always cleared coming into SSA construction, because `TreeRenameVariables` is the only thing that sets them.

JosephTremoulet · 2016-11-04T18:59:47Z

Updated per feedback; @BruceForstall PTAL (and thanks for the suggestions).

briansull · 2016-11-04T20:14:22Z

Looks Good

BruceForstall · 2016-11-04T21:17:14Z

LGTM

dnfclas added the cla-already-signed label Nov 4, 2016

BruceForstall reviewed Nov 4, 2016

View reviewed changes

src/jit/compiler.cpp

if (doEarlyProp)

do

Copy link

Member

BruceForstall Nov 4, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be a while loop so you can set iterations=0?

BruceForstall reviewed Nov 4, 2016

View reviewed changes

JosephTremoulet added 5 commits November 4, 2016 14:57

Clear heap PerSsaData, heapSsaMap in fgResetForSsa

eed63a7

These need to be cleared for the same reasons as the locals' PerSsaData; their omission seems to be a simple oversight.

Clear defnums and post-order nums in fgResetForSsa

50e78c7

SSA construction will overwrite these annotations, but only for reachable code. Clear the annotations in all code in `fgResetForSsa` so that downstream analysis won't stumble over stale/invalid SSA annotations in unreachable code.

Reset EH pred cache in ResetOptAnnotations

7f6a791

The cache created in the first iteration may not be correct in the second. It seems there is a more general problem with failure to invalidate this cache (GitHub issue #7844), that this change is working around for the optRepeat case.

JosephTremoulet force-pushed the OptRepeat branch from edb9da7 to 86d4d59 Compare November 4, 2016 18:59

JosephTremoulet merged commit b67ff5e into dotnet:master Nov 4, 2016

JosephTremoulet deleted the OptRepeat branch November 4, 2016 21:54

karelz modified the milestone: 2.0.0 Aug 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add JitOptRepeat debug config flags #7996

Add JitOptRepeat debug config flags #7996

JosephTremoulet commented Nov 4, 2016

JosephTremoulet commented Nov 4, 2016

BruceForstall Nov 4, 2016

briansull Nov 4, 2016

pgavlin Nov 4, 2016

BruceForstall Nov 4, 2016

BruceForstall Nov 4, 2016

JosephTremoulet Nov 4, 2016

BruceForstall Nov 4, 2016

BruceForstall Nov 4, 2016

BruceForstall Nov 4, 2016

BruceForstall Nov 4, 2016

JosephTremoulet commented Nov 4, 2016

briansull commented Nov 4, 2016

BruceForstall commented Nov 4, 2016


		if (doSsa)
		if (JitConfig.JitOptRepeat().contains(info.compMethodName, info.compClassName, &info.compMethodInfo->args))

Add JitOptRepeat debug config flags #7996

Add JitOptRepeat debug config flags #7996

Conversation

JosephTremoulet commented Nov 4, 2016

JosephTremoulet commented Nov 4, 2016

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

JosephTremoulet commented Nov 4, 2016

briansull commented Nov 4, 2016

BruceForstall commented Nov 4, 2016