Repository navigation
Add JitOptRepeat debug config flags - #7996
Conversation
|
@dotnet/jit-contrib PTAL. Verified no diffs (x64 Windows jit-diff) as-is, and passing DDR with repeat count hard-coded to 16. |
| 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 was a problem hiding this comment.
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.
There was a problem hiding this comment.
+1, Any changes to the jit interface will require more work and it is best to avoid them if possible.
There was a problem hiding this comment.
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.
| #endif | ||
|
|
||
| if (doEarlyProp) | ||
| do |
There was a problem hiding this comment.
Should this be a while loop so you can set iterations=0?
| bool doCopyProp = true; | ||
| bool doAssertionProp = true; | ||
| bool doRangeAnalysis = true; | ||
| int iterations = 1; |
There was a problem hiding this comment.
shouldn't this be unsigned? JitOptRepeatCount can't be specified as negative.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ok. But specifying zero is bad currently -- same as specifying something very large due to "do" loop.
| // in effect. | ||
|
|
||
| void Compiler::ResetOptAnnotations() | ||
| { |
There was a problem hiding this comment.
Can/should you assert that JitOptRepeat is set, and JitOptRepeatCount is >1?
|
|
||
| void Compiler::RecomputeLoopInfo() | ||
| { | ||
| // Recompute reachability sets, dominators, and loops. |
| #endif | ||
|
|
||
| if (doSsa) | ||
| if (JitConfig.JitOptRepeat().contains(info.compMethodName, info.compClassName, &info.compMethodInfo->args)) |
There was a problem hiding this comment.
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.
edb9da7 to
86d4d59
Compare
|
Updated per feedback; @BruceForstall PTAL (and thanks for the suggestions). |
|
Looks Good |
|
LGTM |
6 participants
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.