-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Avoid scanning typeof checks when building whole program view #103883
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -301,12 +301,17 @@ private void ImportBasicBlocks() | |
} | ||
|
||
private void MarkBasicBlock(BasicBlock basicBlock) | ||
{ | ||
MarkBasicBlock(basicBlock, ref _pendingBasicBlocks); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm adding the notion of additional lists that we should look at. This helps in the conditional basic block scanning because it lets us defer looking at conditional blocks until after we scanned all the unconditional codepaths. The conditional scanning is rather simplistic and for: static int Method()
{
if (Foo() == typeof(Bla))
Expensive();
return 0;
} We'd see the Looking at conditional blocks last lets us avoid the rollback (see logic in ILImporter.Scanner.cs). |
||
} | ||
|
||
private static void MarkBasicBlock(BasicBlock basicBlock, ref BasicBlock list) | ||
{ | ||
if (basicBlock.State == BasicBlock.ImportState.Unmarked) | ||
{ | ||
// Link | ||
basicBlock.Next = _pendingBasicBlocks; | ||
_pendingBasicBlocks = basicBlock; | ||
basicBlock.Next = list; | ||
list = basicBlock; | ||
|
||
basicBlock.State = BasicBlock.ImportState.IsPending; | ||
} | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -9,6 +9,8 @@ | |||||
|
||||||
using Debug = System.Diagnostics.Debug; | ||||||
using DependencyList = ILCompiler.DependencyAnalysisFramework.DependencyNodeCore<ILCompiler.DependencyAnalysis.NodeFactory>.DependencyList; | ||||||
using CombinedDependencyList = System.Collections.Generic.List<ILCompiler.DependencyAnalysisFramework.DependencyNodeCore<ILCompiler.DependencyAnalysis.NodeFactory>.CombinedDependencyListEntry>; | ||||||
using DependencyListEntry = ILCompiler.DependencyAnalysisFramework.DependencyNodeCore<ILCompiler.DependencyAnalysis.NodeFactory>.DependencyListEntry; | ||||||
|
||||||
#pragma warning disable IDE0060 | ||||||
|
||||||
|
@@ -28,7 +30,7 @@ internal partial class ILImporter | |||||
|
||||||
private readonly MethodDesc _canonMethod; | ||||||
|
||||||
private DependencyList _dependencies = new DependencyList(); | ||||||
private DependencyList _unconditionalDependencies = new DependencyList(); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
||||||
private readonly byte[] _ilBytes; | ||||||
|
||||||
|
@@ -51,11 +53,17 @@ public enum ImportState : byte | |||||
public bool TryStart; | ||||||
public bool FilterStart; | ||||||
public bool HandlerStart; | ||||||
|
||||||
public object Condition; | ||||||
public DependencyList Dependencies; | ||||||
} | ||||||
|
||||||
private bool _isReadOnly; | ||||||
private TypeDesc _constrained; | ||||||
|
||||||
private DependencyList _dependencies; | ||||||
private BasicBlock _lateBasicBlocks; | ||||||
|
||||||
private sealed class ExceptionRegion | ||||||
{ | ||||||
public ILExceptionRegion ILRegion; | ||||||
|
@@ -107,9 +115,11 @@ public ILImporter(ILScanner compilation, MethodDesc method, MethodIL methodIL = | |||||
{ | ||||||
_exceptionRegions[i] = new ExceptionRegion() { ILRegion = ilExceptionRegions[i] }; | ||||||
} | ||||||
|
||||||
_dependencies = _unconditionalDependencies; | ||||||
} | ||||||
|
||||||
public DependencyList Import() | ||||||
public (DependencyList, CombinedDependencyList) Import() | ||||||
{ | ||||||
TypeDesc owningType = _canonMethod.OwningType; | ||||||
if (_compilation.HasLazyStaticConstructor(owningType)) | ||||||
|
@@ -172,9 +182,21 @@ public DependencyList Import() | |||||
FindBasicBlocks(); | ||||||
ImportBasicBlocks(); | ||||||
|
||||||
CodeBasedDependencyAlgorithm.AddDependenciesDueToMethodCodePresence(ref _dependencies, _factory, _canonMethod, _canonMethodIL); | ||||||
CombinedDependencyList conditionalDependencies = null; | ||||||
foreach (BasicBlock bb in _basicBlocks) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why can there be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This array is indexed into by the IL offset. The entries are non-null at basic block start locations and null elsewhere. |
||||||
{ | ||||||
if (bb?.Condition == null) | ||||||
continue; | ||||||
|
||||||
conditionalDependencies ??= new CombinedDependencyList(); | ||||||
foreach (DependencyListEntry dep in bb.Dependencies) | ||||||
conditionalDependencies.Add(new(dep.Node, bb.Condition, dep.Reason)); | ||||||
} | ||||||
|
||||||
CodeBasedDependencyAlgorithm.AddDependenciesDueToMethodCodePresence(ref _unconditionalDependencies, _factory, _canonMethod, _canonMethodIL); | ||||||
CodeBasedDependencyAlgorithm.AddConditionalDependenciesDueToMethodCodePresence(ref conditionalDependencies, _factory, _canonMethod); | ||||||
|
||||||
return _dependencies; | ||||||
return (_unconditionalDependencies, conditionalDependencies); | ||||||
} | ||||||
|
||||||
private ISymbolNode GetGenericLookupHelper(ReadyToRunHelperId helperId, object helperArgument) | ||||||
|
@@ -199,19 +221,29 @@ private ISymbolNode GetHelperEntrypoint(ReadyToRunHelper helper) | |||||
} | ||||||
|
||||||
private static void MarkInstructionBoundary() { } | ||||||
private static void EndImportingBasicBlock(BasicBlock basicBlock) { } | ||||||
|
||||||
private void EndImportingBasicBlock(BasicBlock basicBlock) | ||||||
{ | ||||||
if (_pendingBasicBlocks == null) | ||||||
{ | ||||||
_pendingBasicBlocks = _lateBasicBlocks; | ||||||
_lateBasicBlocks = null; | ||||||
} | ||||||
} | ||||||
|
||||||
private void StartImportingBasicBlock(BasicBlock basicBlock) | ||||||
{ | ||||||
_dependencies = basicBlock.Condition != null ? basicBlock.Dependencies : _unconditionalDependencies; | ||||||
|
||||||
// Import all associated EH regions | ||||||
foreach (ExceptionRegion ehRegion in _exceptionRegions) | ||||||
{ | ||||||
ILExceptionRegion region = ehRegion.ILRegion; | ||||||
if (region.TryOffset == basicBlock.StartOffset) | ||||||
{ | ||||||
MarkBasicBlock(_basicBlocks[region.HandlerOffset]); | ||||||
ImportBasicBlockEdge(basicBlock, _basicBlocks[region.HandlerOffset]); | ||||||
if (region.Kind == ILExceptionRegionKind.Filter) | ||||||
MarkBasicBlock(_basicBlocks[region.FilterOffset]); | ||||||
ImportBasicBlockEdge(basicBlock, _basicBlocks[region.FilterOffset]); | ||||||
|
||||||
if (region.Kind == ILExceptionRegionKind.Catch) | ||||||
{ | ||||||
|
@@ -789,10 +821,26 @@ private void ImportCalli(int token) | |||||
|
||||||
private void ImportBranch(ILOpcode opcode, BasicBlock target, BasicBlock fallthrough) | ||||||
{ | ||||||
object condition = null; | ||||||
|
||||||
if (opcode == ILOpcode.brfalse | ||||||
&& _typeEqualityPatternAnalyzer.IsTypeEqualityBranch | ||||||
&& !_typeEqualityPatternAnalyzer.IsTwoTokens | ||||||
&& !_typeEqualityPatternAnalyzer.IsInequality) | ||||||
{ | ||||||
TypeDesc typeEqualityCheckType = (TypeDesc)_canonMethodIL.GetObject(_typeEqualityPatternAnalyzer.Token1); | ||||||
if (!typeEqualityCheckType.IsGenericDefinition | ||||||
&& ConstructedEETypeNode.CreationAllowed(typeEqualityCheckType) | ||||||
&& !typeEqualityCheckType.ConvertToCanonForm(CanonicalFormKind.Specific).IsCanonicalSubtype(CanonicalFormKind.Any)) | ||||||
{ | ||||||
condition = _factory.MaximallyConstructableType(typeEqualityCheckType); | ||||||
} | ||||||
} | ||||||
|
||||||
ImportFallthrough(target); | ||||||
|
||||||
if (fallthrough != null) | ||||||
ImportFallthrough(fallthrough); | ||||||
ImportFallthrough(fallthrough, condition); | ||||||
} | ||||||
|
||||||
private void ImportSwitchJump(int jmpBase, int[] jmpDelta, BasicBlock fallthrough) | ||||||
|
@@ -1278,9 +1326,56 @@ private void ImportConvert(WellKnownType wellKnownType, bool checkOverflow, bool | |||||
} | ||||||
} | ||||||
|
||||||
private void ImportFallthrough(BasicBlock next) | ||||||
private void ImportBasicBlockEdge(BasicBlock source, BasicBlock next, object condition = null) | ||||||
{ | ||||||
// We don't track multiple conditions; if the source basic block is only reachable under a condition, | ||||||
// this condition will be used for the next basic block, irrespective if we could make it more narrow. | ||||||
object effectiveCondition = source.Condition ?? condition; | ||||||
|
||||||
// Did we already look at this basic block? | ||||||
if (next.State != BasicBlock.ImportState.Unmarked) | ||||||
{ | ||||||
// If next is not conditioned, it stays not conditioned. | ||||||
// If it was conditioned on something else, it needs to become unconditional. | ||||||
// If the conditions match, it stays conditioned on the same thing. | ||||||
if (next.Condition != null && next.Condition != effectiveCondition) | ||||||
{ | ||||||
// Now we need to make `next` not conditioned. We move all of its dependencies to | ||||||
// unconditional dependencies, and do this for all basic blocks that are reachable | ||||||
// from it. | ||||||
// TODO-SIZE: below doesn't do it for all basic blocks reachable from `next`, but | ||||||
// for all basic blocks with the same conditon. This is a shortcut. It likely | ||||||
// doesn't matter in practice. | ||||||
object conditionToRemove = next.Condition; | ||||||
foreach (BasicBlock bb in _basicBlocks) | ||||||
{ | ||||||
if (bb?.Condition == conditionToRemove) | ||||||
{ | ||||||
_unconditionalDependencies.AddRange(bb.Dependencies); | ||||||
bb.Dependencies = null; | ||||||
bb.Condition = null; | ||||||
} | ||||||
} | ||||||
} | ||||||
} | ||||||
else | ||||||
{ | ||||||
if (effectiveCondition != null) | ||||||
{ | ||||||
next.Condition = effectiveCondition; | ||||||
next.Dependencies = new DependencyList(); | ||||||
MarkBasicBlock(next, ref _lateBasicBlocks); | ||||||
} | ||||||
else | ||||||
{ | ||||||
MarkBasicBlock(next); | ||||||
} | ||||||
} | ||||||
} | ||||||
|
||||||
private void ImportFallthrough(BasicBlock next, object condition = null) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: to me "fallthrough" means "the case where the condition wasn't satisfied (or there was no condition)", so I'd expect condition to always be null. Maybe add a separate helper that accepts a condition? I see the existing code uses ImportFallthrough for branch targets too, so feel free to leave as-is if you prefer. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it's weird, it's not my naming, but I don't have a better name, so I'll not churn CI for this. |
||||||
{ | ||||||
MarkBasicBlock(next); | ||||||
ImportBasicBlockEdge(_currentBasicBlock, next, condition); | ||||||
} | ||||||
|
||||||
private int ReadILTokenAt(int ilOffset) | ||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I didn't miss any cases, this can only be ReadyToRunGenericHelperNode, MakeGenericMethodSite, or MakeGenericTypeSite. How does this work for methods on generic types that are instantiated directly rather than through MakeGenericType?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think #103883 (comment) explains it, but skips a small step. I wrote "Canonical method
Blah
depends on e.g. T__Canon (for thetypeof(T)
).", but the actual dependency is: "Canonical methodBlah
depends on ReadyToRunGenericHelperNode of T__Canon."So this is how directly instantiated things are tracked. The remaining MakeGenericMethodSite and MakeGenericTypeSite are relatively recent additions from #99037 and are only used in relation to dataflow since the problem is similar (we compute something on a "lesser instantiated thing" and need to specialize it for whatever else is in the dependency graph).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!