-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Assertion failed 'argInfo.argIsUsed == false' during 'Morph - Inlining' #112092
Comments
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
Do you mean adding But I don't get how can this affect |
I meant speed (the comment has both size and speed, but this is for speed). Speed enables tier-1 inliner and will inline more. |
I've created #112097 and running outerloop on it just to check the theory. |
I can repro this locally. While I didn't see any fault of #111948, it's doing the right thing.
into
Then we are trying to inline it, so that we split the tree
into
Then it managed to inline it, and resulted in the following tree:
Then the inliner tries to inline
So far everything is fine. Then the inliner tries to inline
And the inlinee's tree containing
@jakobbotsch I don't think this issue is introduced by my change, without #111948 we would not try to inline BTW it seems that it doesn't affect the correctness. A repro is using System.Collections.Concurrent;
using System.Runtime.CompilerServices;
new MyPartitioner<string>([]).GetDynamicPartitions();
public class MyPartitioner<TSource> : Partitioner<TSource>
{
private IList<TSource> _data;
public MyPartitioner(IList<TSource> data)
{
_data = data;
}
public override IList<IEnumerator<TSource>> GetPartitions(int partitionCount)
{
if (partitionCount <= 0)
{
throw new ArgumentOutOfRangeException(nameof(partitionCount));
}
IEnumerator<TSource>[] partitions
= new IEnumerator<TSource>[partitionCount];
IEnumerable<KeyValuePair<long, TSource>> partitionEnumerable = Partitioner.Create(_data, true).GetOrderableDynamicPartitions();
for (int i = 0; i < partitionCount; i++)
{
partitions[i] = DropIndices(partitionEnumerable.GetEnumerator());
}
return partitions;
}
[MethodImpl(MethodImplOptions.NoInlining)]
public override IEnumerable<TSource> GetDynamicPartitions()
{
return DropIndices(Partitioner.Create(_data, true).GetOrderableDynamicPartitions());
}
private static IEnumerable<TSource> DropIndices(IEnumerable<KeyValuePair<long, TSource>> source)
{
foreach (KeyValuePair<long, TSource> pair in source)
{
yield return pair.Value;
}
}
private static IEnumerator<TSource> DropIndices(IEnumerator<KeyValuePair<long, TSource>> source)
{
while (source.MoveNext())
{
yield return source.Current.Value;
}
}
public override bool SupportsDynamicPartitions
{
get { return true; }
}
} |
My guess is that since |
So the fix seems to be: if we split an arg of a call that is already an inline candidate, we should update its inline info? |
Maybe. I don't know if that is feasible. It might be easier to just skip late inlining for cases where the parent is already an inline candidate. Apparently, this doesn't happen all that often or we would have caught this in the testing we did before merging. |
I think it will be fixed if we switch the order of inlining and |
Hmm, I guess we really want to do the substitution of |
We could notice during the substitution if we form new inlining candidates and re-walk to do those inlines, perhaps. |
runtime/src/coreclr/jit/importer.cpp Lines 13143 to 13148 in ca99c80
And seems that we are conservatively marking all non-constant inlinee args which are inst param as |
That's just for |
I can confirm #112010 fixes this issue. /cc @MichalStrehovsky, the fix seems to be #112010. |
I think the underlying problem still remains. It is not safe to modify a call's arguments once we have formed the inline candidate info. |
Though presumably we do exactly this when we substitute for ret exprs; perhaps that works because those look like calls during the initial analysis and so get "worst case" treatement (that is they have no special properties we can exploit). |
The inline candidate info seems to contain nothing about the call's arguments. We don't form the inlinee's arg info until we are actually inlining it. My thought is that the issue is about an edge case when it comes to an |
Ah, ok, right, we don't gather the inline arg info early. That makes sense. So maybe the issue is just restricted to instance params. |
Then let me grab this one. #112010 still needs a bit of work to investigate some regressions. |
* Handle InstParam in the same way as all other arguments * Insert the evaluation of the InstParam in the right spot when inlining succeeded Fix dotnet#112092
Sample failure log: https://dev.azure.com/dnceng-public/public/_build/results?buildId=937252&view=logs&j=6a7e26fa-36e7-5a45-28af-dc6c8e6724e6&t=089ef86b-599a-543a-2c8c-82b31601e3ec
This is failing in native AOT
OptimizationPreference=Speed
runs on both Windows and Linux. I.e. you need to add/p:OptimizationPreference=Speed
to the build command line, in addition to/p:TestNativeAot=true
.This started failing sometime between ad7b829 (last good run) and 6f221b4 (first bad run).
The text was updated successfully, but these errors were encountered: