Skip to content
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.

Sign up for GitHub

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

Fix trim analysis for assignment to multiple refs in single method call #105663

Merged
merged 10 commits into from
Aug 8, 2024
Merged

Fix trim analysis for assignment to multiple refs in single method call #105663

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
1d629ac
Add test
sbomer Jul 29, 2024
3105c37
Fix linker behavior
sbomer Jul 29, 2024
f542aa7
Fix ILC behavior
sbomer Jul 29, 2024
0447405
Clean up analyzer implementation
sbomer Jul 29, 2024
eafaa6d
Fix linker behavior
sbomer Jul 29, 2024
a4c0fb8
Fix ILC behavior
sbomer Jul 29, 2024
2ee0ffe
Update test
sbomer Jul 29, 2024
c9bde7a
Merge remote-tracking branch 'origin/main' into multipleOutRefs
sbomer Aug 7, 2024
5320ffe
Merge remote-tracking branch 'origin/main' into multipleOutRefs
sbomer Aug 7, 2024
f7536b3
Cleanup
sbomer Aug 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 10 additions & 10 deletions src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/MethodBodyScanner.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -901,7 +901,7 @@ Stack<StackSlot> currentStack
ParameterProxy param = new(methodBody.OwningMethod, paramNum);
var targetValue = GetMethodParameterValue(param);
if (targetValue is MethodParameterValue targetParameterValue)
HandleStoreParameter(methodBody, offset, targetParameterValue, valueToStore.Value);
HandleStoreParameter(methodBody, offset, targetParameterValue, valueToStore.Value, null);

// If the targetValue is MethodThisValue do nothing - it should never happen really, and if it does, there's nothing we can track there
}
Expand Down Expand Up @@ -1007,7 +1007,7 @@ private void ScanIndirectStore(
StackSlot valueToStore = PopUnknown(currentStack, 1, methodBody, offset);
StackSlot destination = PopUnknown(currentStack, 1, methodBody, offset);

StoreInReference(destination.Value, valueToStore.Value, methodBody, offset, locals, curBasicBlock, ref ipState);
StoreInReference(destination.Value, valueToStore.Value, methodBody, offset, locals, curBasicBlock, ref ipState, null);
}

/// <summary>
Expand All @@ -1018,7 +1018,7 @@ private void ScanIndirectStore(
/// <param name="method">The method body that contains the operation causing the store</param>
/// <param name="offset">The instruction offset causing the store</param>
/// <exception cref="LinkerFatalErrorException">Throws if <paramref name="target"/> is not a valid target for an indirect store.</exception>
protected void StoreInReference(MultiValue target, MultiValue source, MethodIL method, int offset, ValueBasicBlockPair?[] locals, int curBasicBlock, ref InterproceduralState ipState)
protected void StoreInReference(MultiValue target, MultiValue source, MethodIL method, int offset, ValueBasicBlockPair?[] locals, int curBasicBlock, ref InterproceduralState ipState, int? parameterIndex)
{
foreach (var value in target.AsEnumerable ())
{
Expand All @@ -1029,18 +1029,18 @@ protected void StoreInReference(MultiValue target, MultiValue source, MethodIL m
break;
case FieldReferenceValue fieldReference
when HandleGetField(method, offset, fieldReference.FieldDefinition).AsSingleValue() is FieldValue fieldValue:
HandleStoreField(method, offset, fieldValue, source);
HandleStoreField(method, offset, fieldValue, source, parameterIndex);
break;
case ParameterReferenceValue parameterReference
when GetMethodParameterValue(parameterReference.Parameter) is MethodParameterValue parameterValue:
HandleStoreParameter(method, offset, parameterValue, source);
HandleStoreParameter(method, offset, parameterValue, source, parameterIndex);
break;
case MethodReturnValue methodReturnValue:
// Ref returns don't have special ReferenceValue values, so assume if the target here is a MethodReturnValue then it must be a ref return value
HandleReturnValue(method, offset, methodReturnValue, source);
break;
case FieldValue fieldValue:
HandleStoreField(method, offset, fieldValue, DereferenceValue(method, offset, source, locals, ref ipState));
HandleStoreField(method, offset, fieldValue, DereferenceValue(method, offset, source, locals, ref ipState), parameterIndex);
break;
case IValueWithStaticType valueWithStaticType:
if (valueWithStaticType.StaticType is not null && FlowAnnotations.IsTypeInterestingForDataflow(valueWithStaticType.StaticType.Value.Type))
Expand Down Expand Up @@ -1108,11 +1108,11 @@ private void ScanLdfld(
currentStack.Push(new StackSlot(value));
}

protected virtual void HandleStoreField(MethodIL method, int offset, FieldValue field, MultiValue valueToStore)
protected virtual void HandleStoreField(MethodIL method, int offset, FieldValue field, MultiValue valueToStore, int? parameterIndex)
{
}

protected virtual void HandleStoreParameter(MethodIL method, int offset, MethodParameterValue parameter, MultiValue valueToStore)
protected virtual void HandleStoreParameter(MethodIL method, int offset, MethodParameterValue parameter, MultiValue valueToStore, int? parameterIndex)
{
}

Expand Down Expand Up @@ -1149,7 +1149,7 @@ private void ScanStfld(
// Incomplete handling of ref fields -- if we're storing a reference to a value, pretend it's just the value
MultiValue valueToStore = DereferenceValue(methodBody, offset, valueToStoreSlot.Value, locals, ref interproceduralState);

HandleStoreField(methodBody, offset, fieldValue, valueToStore);
HandleStoreField(methodBody, offset, fieldValue, valueToStore, null);
}
}

Expand Down Expand Up @@ -1241,7 +1241,7 @@ protected void AssignRefAndOutParameters(
if (parameter.GetReferenceKind() is not (ReferenceKind.Ref or ReferenceKind.Out))
continue;
var newByRefValue = _annotations.GetMethodParameterValue(parameter);
StoreInReference(methodArguments[(int)parameter.Index], newByRefValue, callingMethodBody, offset, locals, curBasicBlock, ref ipState);
StoreInReference(methodArguments[(int)parameter.Index], newByRefValue, callingMethodBody, offset, locals, curBasicBlock, ref ipState, parameter.Index.Index);
}
}

Expand Down
14 changes: 7 additions & 7 deletions src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMethodBodyScanner.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,23 +208,23 @@ protected override MultiValue HandleGetField(MethodIL methodBody, int offset, Fi
return _annotations.GetFieldValue(field);
}

private void HandleStoreValueWithDynamicallyAccessedMembers(MethodIL methodBody, int offset, ValueWithDynamicallyAccessedMembers targetValue, MultiValue sourceValue, string reason)
private void HandleStoreValueWithDynamicallyAccessedMembers(MethodIL methodBody, int offset, ValueWithDynamicallyAccessedMembers targetValue, MultiValue sourceValue, int? parameterIndex, string reason)
{
if (targetValue.DynamicallyAccessedMemberTypes != 0)
{
_origin = _origin.WithInstructionOffset(methodBody, offset);
TrimAnalysisPatterns.Add(new TrimAnalysisAssignmentPattern(sourceValue, targetValue, _origin, reason));
TrimAnalysisPatterns.Add(new TrimAnalysisAssignmentPattern(sourceValue, targetValue, _origin, parameterIndex, reason));
}
}

protected override void HandleStoreField(MethodIL methodBody, int offset, FieldValue field, MultiValue valueToStore)
=> HandleStoreValueWithDynamicallyAccessedMembers(methodBody, offset, field, valueToStore, field.Field.GetDisplayName());
protected override void HandleStoreField(MethodIL methodBody, int offset, FieldValue field, MultiValue valueToStore, int? parameterIndex)
=> HandleStoreValueWithDynamicallyAccessedMembers(methodBody, offset, field, valueToStore, parameterIndex, field.Field.GetDisplayName());

protected override void HandleStoreParameter(MethodIL methodBody, int offset, MethodParameterValue parameter, MultiValue valueToStore)
=> HandleStoreValueWithDynamicallyAccessedMembers(methodBody, offset, parameter, valueToStore, parameter.Parameter.Method.GetDisplayName());
protected override void HandleStoreParameter(MethodIL methodBody, int offset, MethodParameterValue parameter, MultiValue valueToStore, int? parameterIndex)
=> HandleStoreValueWithDynamicallyAccessedMembers(methodBody, offset, parameter, valueToStore, parameterIndex, parameter.Parameter.Method.GetDisplayName());

protected override void HandleReturnValue(MethodIL methodBody, int offset, MethodReturnValue returnValue, MultiValue valueToStore)
=> HandleStoreValueWithDynamicallyAccessedMembers(methodBody, offset, returnValue, valueToStore, returnValue.Method.GetDisplayName());
=> HandleStoreValueWithDynamicallyAccessedMembers(methodBody, offset, returnValue, valueToStore, null, returnValue.Method.GetDisplayName());

protected override void HandleTypeTokenAccess(MethodIL methodBody, int offset, TypeDesc accessedType)
{
Expand Down
9 changes: 8 additions & 1 deletion src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/TrimAnalysisAssignmentPattern.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,24 +18,31 @@ public readonly record struct TrimAnalysisAssignmentPattern
public MultiValue Source { get; init; }
public MultiValue Target { get; init; }
public MessageOrigin Origin { get; init; }

// For assignment of a method parameter, we store the parameter index to disambiguate
// assignments from different out parameters of a single method call.
public int? ParameterIndex { get; init; }
internal string Reason { get; init; }

internal TrimAnalysisAssignmentPattern(MultiValue source, MultiValue target, MessageOrigin origin, string reason)
internal TrimAnalysisAssignmentPattern(MultiValue source, MultiValue target, MessageOrigin origin, int? parameterIndex, string reason)
{
Source = source.DeepCopy();
Target = target.DeepCopy();
Origin = origin;
ParameterIndex = parameterIndex;
Reason = reason;
}

public TrimAnalysisAssignmentPattern Merge(ValueSetLattice<SingleValue> lattice, TrimAnalysisAssignmentPattern other)
{
Debug.Assert(Origin == other.Origin);
Debug.Assert(ParameterIndex == other.ParameterIndex);

return new TrimAnalysisAssignmentPattern(
lattice.Meet(Source, other.Source),
lattice.Meet(Target, other.Target),
Origin,
ParameterIndex,
Reason);
}

Expand Down
11 changes: 6 additions & 5 deletions src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/TrimAnalysisPatternStore.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ namespace ILCompiler.Dataflow
{
public readonly struct TrimAnalysisPatternStore
{
private readonly Dictionary<MessageOrigin, TrimAnalysisAssignmentPattern> AssignmentPatterns;
private readonly Dictionary<(MessageOrigin, int?), TrimAnalysisAssignmentPattern> AssignmentPatterns;
private readonly Dictionary<MessageOrigin, TrimAnalysisMethodCallPattern> MethodCallPatterns;
private readonly Dictionary<(MessageOrigin, TypeSystemEntity), TrimAnalysisTokenAccessPattern> TokenAccessPatterns;
private readonly Dictionary<(MessageOrigin, TypeSystemEntity), TrimAnalysisGenericInstantiationAccessPattern> GenericInstantiations;
Expand All @@ -23,7 +23,7 @@ public readonly struct TrimAnalysisPatternStore

public TrimAnalysisPatternStore(ValueSetLattice<SingleValue> lattice, Logger logger)
{
AssignmentPatterns = new Dictionary<MessageOrigin, TrimAnalysisAssignmentPattern>();
AssignmentPatterns = new Dictionary<(MessageOrigin, int?), TrimAnalysisAssignmentPattern>();
MethodCallPatterns = new Dictionary<MessageOrigin, TrimAnalysisMethodCallPattern>();
TokenAccessPatterns = new Dictionary<(MessageOrigin, TypeSystemEntity), TrimAnalysisTokenAccessPattern>();
GenericInstantiations = new Dictionary<(MessageOrigin, TypeSystemEntity), TrimAnalysisGenericInstantiationAccessPattern>();
Expand All @@ -34,13 +34,14 @@ public TrimAnalysisPatternStore(ValueSetLattice<SingleValue> lattice, Logger log

public void Add(TrimAnalysisAssignmentPattern pattern)
{
if (!AssignmentPatterns.TryGetValue(pattern.Origin, out var existingPattern))
var key = (pattern.Origin, pattern.ParameterIndex);
if (!AssignmentPatterns.TryGetValue(key, out var existingPattern))
{
AssignmentPatterns.Add(pattern.Origin, pattern);
AssignmentPatterns.Add(key, pattern);
return;
}

AssignmentPatterns[pattern.Origin] = pattern.Merge(Lattice, existingPattern);
AssignmentPatterns[key] = pattern.Merge(Lattice, existingPattern);
}

public void Add(TrimAnalysisMethodCallPattern pattern)
Expand Down
27 changes: 15 additions & 12 deletions src/tools/illink/src/linker/Linker.Dataflow/MethodBodyScanner.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -750,7 +750,7 @@ private void ScanStarg (
ParameterProxy param = new (thisMethod, paramNum);
var targetValue = GetMethodParameterValue (param);
if (targetValue is MethodParameterValue targetParameterValue)
HandleStoreParameter (thisMethod, targetParameterValue, operation, valueToStore.Value);
HandleStoreParameter (thisMethod, targetParameterValue, operation, valueToStore.Value, null);

// If the targetValue is MethodThisValue do nothing - it should never happen really, and if it does, there's nothing we can track there
}
Expand Down Expand Up @@ -846,7 +846,7 @@ private void ScanIndirectStore (
StackSlot valueToStore = PopUnknown (currentStack, 1, methodBody, operation.Offset);
StackSlot destination = PopUnknown (currentStack, 1, methodBody, operation.Offset);

StoreInReference (destination.Value, valueToStore.Value, methodBody.Method, operation, locals, curBasicBlock, ref ipState);
StoreInReference (destination.Value, valueToStore.Value, methodBody.Method, operation, locals, curBasicBlock, ref ipState, null);
}

/// <summary>
Expand All @@ -856,8 +856,9 @@ private void ScanIndirectStore (
/// <param name="source">The value to store</param>
/// <param name="method">The method body that contains the operation causing the store</param>
/// <param name="operation">The instruction causing the store</param>
/// <param name="parameterIndex">For assignment due to a call to a method with out params, the index of the out parameter.</param>
/// <exception cref="LinkerFatalErrorException">Throws if <paramref name="target"/> is not a valid target for an indirect store.</exception>
protected void StoreInReference (MultiValue target, MultiValue source, MethodDefinition method, Instruction operation, LocalVariableStore locals, int curBasicBlock, ref InterproceduralState ipState)
protected void StoreInReference (MultiValue target, MultiValue source, MethodDefinition method, Instruction operation, LocalVariableStore locals, int curBasicBlock, ref InterproceduralState ipState, int? parameterIndex)
{
foreach (var value in target.AsEnumerable ()) {
switch (value) {
Expand All @@ -866,18 +867,18 @@ protected void StoreInReference (MultiValue target, MultiValue source, MethodDef
break;
case FieldReferenceValue fieldReference
when GetFieldValue (fieldReference.FieldDefinition).AsSingleValue () is FieldValue fieldValue:
HandleStoreField (method, fieldValue, operation, source);
HandleStoreField (method, fieldValue, operation, source, parameterIndex);
break;
case ParameterReferenceValue parameterReference
when GetMethodParameterValue (parameterReference.Parameter) is MethodParameterValue parameterValue:
HandleStoreParameter (method, parameterValue, operation, source);
HandleStoreParameter (method, parameterValue, operation, source, parameterIndex);
break;
case MethodReturnValue methodReturnValue:
// Ref returns don't have special ReferenceValue values, so assume if the target here is a MethodReturnValue then it must be a ref return value
HandleReturnValue (method, methodReturnValue, operation, source);
break;
case FieldValue fieldValue:
HandleStoreField (method, fieldValue, operation, DereferenceValue (source, locals, ref ipState));
HandleStoreField (method, fieldValue, operation, DereferenceValue (source, locals, ref ipState), parameterIndex);
break;
case IValueWithStaticType valueWithStaticType:
if (valueWithStaticType.StaticType is not null && _context.Annotations.FlowAnnotations.IsTypeInterestingForDataflow (valueWithStaticType.StaticType.Value.Type))
Expand Down Expand Up @@ -927,11 +928,11 @@ private void ScanLdfld (
currentStack.Push (new StackSlot (value));
}

protected virtual void HandleStoreField (MethodDefinition method, FieldValue field, Instruction operation, MultiValue valueToStore)
protected virtual void HandleStoreField (MethodDefinition method, FieldValue field, Instruction operation, MultiValue valueToStore, int? parameterIndex)
{
}

protected virtual void HandleStoreParameter (MethodDefinition method, MethodParameterValue parameter, Instruction operation, MultiValue valueToStore)
protected virtual void HandleStoreParameter (MethodDefinition method, MethodParameterValue parameter, Instruction operation, MultiValue valueToStore, int? parameterIndex)
{
}

Expand Down Expand Up @@ -967,7 +968,7 @@ private void ScanStfld (
// Incomplete handling of ref fields -- if we're storing a reference to a value, pretend it's just the value
MultiValue valueToStore = DereferenceValue (valueToStoreSlot.Value, locals, ref interproceduralState);

HandleStoreField (thisMethod, fieldValue, operation, valueToStore);
HandleStoreField (thisMethod, fieldValue, operation, valueToStore, null);
}
}
}
Expand Down Expand Up @@ -1062,15 +1063,17 @@ protected void AssignRefAndOutParameters (
if (parameter.GetReferenceKind () is not (ReferenceKind.Ref or ReferenceKind.Out))
continue;
var newByRefValue = _context.Annotations.FlowAnnotations.GetMethodParameterValue (parameter);
StoreInReference (methodArguments[(int) parameter.Index], newByRefValue, callingMethodBody.Method, operation, locals, curBasicBlock, ref ipState);
StoreInReference (methodArguments[(int) parameter.Index], newByRefValue, callingMethodBody.Method, operation, locals, curBasicBlock, ref ipState, parameter.Index.Index);
}
} else {
// We couldn't resolve the method, so we put unknown values into the ref and out arguments
// Should be a very cold path, so using Linq.Zip should be okay
foreach (var (argument, refKind) in methodArguments.Zip (calledMethod.GetParameterReferenceKinds ())) {
var argumentRefKinds = methodArguments.Zip (calledMethod.GetParameterReferenceKinds ()).ToList ();
for (int index = 0; index < argumentRefKinds.Count; index++) {
var (argument, refKind) = argumentRefKinds[index];
if (refKind is not (ReferenceKind.Ref or ReferenceKind.Out))
continue;
StoreInReference (argument, UnknownValue.Instance, callingMethodBody.Method, operation, locals, curBasicBlock, ref ipState);
StoreInReference (argument, UnknownValue.Instance, callingMethodBody.Method, operation, locals, curBasicBlock, ref ipState, index);
}
}
}
Expand Down
Loading
Loading