You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
JIT: Bad codegen on x86 for a function in collectible ALC #73951
// Generated by Fuzzlyn v1.5 on 2022-08-15 13:32:00// Run on X86 Windows// Seed: 9809551563097583633// Reduced from 536.2 KiB to 1.8 KiB in 00:20:04// Debug: Outputs 0// Release: Outputs 61952publicstructS0{publicuintF1;publicintF2;publicbyteF3;}publicclassC0{publicushortF1;publicC0(ushortf1){F1=f1;}}publicclassC1{publicC1(uintf0,uintf1){}}publicclassC2{publicC2(C1f0,uintf2,ulongf3,boolf5,intf6){}}publicstructS1{publicC2F0;publicS0F1;publicS1(C2f0):this(){F0=f0;}publicS1M105(S0arg0){Program.s_10.F0=newC2(newC1(521025157U,1254134592U),2911867824U,0,false,0);returnthis;}}publicclassProgram{publicstaticIRuntimes_rt;publicstaticS1s_10;publicstaticS1s_17;publicstaticvoidMain(){CollectibleALCalc=newCollectibleALC();System.Reflection.Assemblyasm=alc.LoadFromAssemblyPath(System.Reflection.Assembly.GetExecutingAssembly().Location);System.Reflection.MethodInfomi=asm.GetType(typeof(Program).FullName).GetMethod(nameof(MainInner));System.TyperuntimeTy=asm.GetType(typeof(Runtime).FullName);mi.Invoke(null,newobject[]{System.Activator.CreateInstance(runtimeTy)});}publicstaticvoidMainInner(IRuntimert){s_rt=rt;S0vr21=s_17.F1;newS1(newC2(newC1(0,0),0,0,false,0)).M105(vr21);varvr22=newC0(vr21.F3);s_rt.WriteLine("c_1839",vr22.F1);}}publicinterfaceIRuntime{voidWriteLine<T>(stringsite,Tvalue);}publicclassRuntime:IRuntime{publicvoidWriteLine<T>(stringsite,Tvalue)=>System.Console.WriteLine(value);}publicclassCollectibleALC:System.Runtime.Loader.AssemblyLoadContext{publicCollectibleALC():base(true){}}
The assumption being made is that we're only altering the padding contents and those should not matter. Clearly, there is a logic error there somewhere.
Edit: note we had this logic in codegen even before #65803.
I see what is going on. I suppose it should have been obvious from the start...
IN0018: 000072movesi, dword ptr [V13 ebp-48H] // Denormalized value loaded (recall V13 is "ubyte")IN0019: 000075pushesi // This use doesn't care...IN0022: 000094mov word ptr [edi+4],si // This one does
The implication is that this kind of retyping is not legal for candidate locals.
Will be preparing fixes tomorrow; they may take some time due to this code relying in the retyping a good deal (remember that the whole thing is intended to make pushing thing easier).
Edit: this is also an interesting case from a perspective of normalize-on-load semantics. We remove all of the normalizing casts because the def of the small field local is itself a small value. One could view the bug as the "normalize-on-load locals can only appear without the corresponding casts if typed small" rule being violated.
Activity
jakobbotsch commentedon Aug 15, 2022
Looks like we fail to normalize
vr21.F3when we constructvr22:Full disassembly:
JulieLeeMSFT commentedon Aug 15, 2022
@jakobbotsch, putting this to .NET 8. Please change to .NET 7 if it needs to be fixed in this release.
jakobbotsch commentedon Aug 15, 2022
This seems like an an interaction between spilling/unspilling and this transformation:
runtime/src/coreclr/jit/lowerxarch.cpp
Lines 510 to 520 in 0a2b4eb
This was added in #65803 from what I can see, cc @SingleAccretion
SingleAccretion commentedon Aug 15, 2022
Looking at this already.
The assumption being made is that we're only altering the padding contents and those should not matter. Clearly, there is a logic error there somewhere.
Edit: note we had this logic in codegen even before #65803.
SingleAccretion commentedon Aug 15, 2022
I see what is going on. I suppose it should have been obvious from the start...
The implication is that this kind of retyping is not legal for candidate locals.
Will be preparing fixes tomorrow; they may take some time due to this code relying in the retyping a good deal (remember that the whole thing is intended to make
pushing thing easier).Edit: this is also an interesting case from a perspective of normalize-on-load semantics. We remove all of the normalizing casts because the def of the small field local is itself a small value. One could view the bug as the "normalize-on-load locals can only appear without the corresponding casts if typed small" rule being violated.