[QualityWeek] Check-Release diffs with x86 #58063
Comments
dotnet-issue-labeler
bot
added
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT Issue DetailsSeven methods show Check-Release diffs with x86 (at b8a5a4a, jit guid c2e4a466-795d-4e64-a776-0ae7b2eed65b) In windows.x86\libraries.pmi.windows.x86.checked.mch
In windows.x86\libraries_tests.pmi.windows.x86.checked.mch
Sample diff (322112) ;;; Check
8b bd 5c ff ff ff mov edi, dword ptr [ebp - 164]
8b 8d 60 ff ff ff mov ecx, dword ptr [ebp - 160]
8b 95 58 ff ff ff mov edx, dword ptr [ebp - 168]
89 95 54 ff ff ff mov dword ptr [ebp - 172], edx
8b 55 94 mov edx, dword ptr [ebp - 108]
53 push ebx
57 push edi
56 push esi
51 push ecx
50 push eax
52 push edx
ff b5 54 ff ff ff push dword ptr [ebp - 172]
;;; Release
89 85 7c ff ff ff mov dword ptr [ebp - 132], eax
8b bd 58 ff ff ff mov edi, dword ptr [ebp - 168]
8b 8d 5c ff ff ff mov ecx, dword ptr [ebp - 164]
8b 95 54 ff ff ff mov edx, dword ptr [ebp - 172]
8b 45 94 mov eax, dword ptr [ebp - 108]
53 push ebx
57 push edi
56 push esi
51 push ecx
8b 8d 7c ff ff ff mov ecx, dword ptr [ebp - 132]
51 push ecx
50 push eax
52 push edxSample repro:
|
AndyAyersMS
removed
the
untriaged
|
CC @dotnet/jit-contrib |
|
There was also one method with diffs on arm64 both with windows and linux, but I suspect it's the spurious SPMI static alignment thing (#53773). |
|
@dotnet/jit-contrib These can be tricky to track down. Anyone up for a challenge? |
|
I can give it a go (I have no 6.0 issues as well). |
|
I'm not able to reproduce this. On b8a5a4a, even replaying all of .\artifacts\tests\coreclr\windows.x86.Checked\Tests\Core_Root\superpmi.exe -a .\artifacts\tests\coreclr\windows.x86.Checked\Tests\Core_Root\clrjit.dll .\artifacts\tests\coreclr\windows.x86.Release\Tests\Core_Root\clrjit.dll "D:\dev\dotnet\spmi\mch\c2e4a466-795d-4e64-a776-0ae7b2eed65b.windows.x86\libraries_tests.pmi.windows.x86.checked.mch"giving me Weirdly, the code size I get for 322112 is also significantly different from either of the code sizes you get: |
|
For libraries.pmi.windows.x86.checked.mch I found a GC info diff for -c 127268 that I am looking at now. |
|
I can still repro these...maybe we have slightly different collections? Here are the method names so you can cross-check. Release jit will show zero method size which is why these look like improvements. libraries.pmi.windows.x86.checked.mch:Detail diffslibraries_tests.pmi.windows.x86.checked.mch:Detail diffsMy file sizes are: |
|
Indeed, looks like our collections are different. Strange that I didn't find the diffs with a full diff, though. The GC info problem might be the cause for the other diffs as well -- two tracked locals get |
|
I believe the weight is the block weight, so trying to track down why those are different now. In the end, we might need to add an epsilon to this check: runtime/src/coreclr/jit/lclvars.cpp Lines 3286 to 3289 in 3f61d42 |
|
Did you just pull down the SPMI collections? I can try sharing out my versions and filter them down to just the implicated MCs so they're not so massive. |
|
No, last time I pulled it was after #57306 so they are a bit old. I'll try updating the collection after I'm done with that other issue. |
|
fwiw, #51317 is a proposal to avoid this problem of "when did you pull down the collections?" and the fact we currently overwrite collections in storage every weekend. |
|
MCHs with just those 7 failing methods here: https://gist.github.com/AndyAyersMS/6055fa9ec2062385ec3d60a9e2ebe78d |
|
The issue seems to be that we are using x87 in checked builds while we are using SSE in release builds. The code generated for In checked, the code generated by MSVC is // getBBWeight -- get the normalized weight of this block
BasicBlock::weight_t BasicBlock::getBBWeight(Compiler* comp)
{
6A8E6B65 push ebp
6A8E6B66 mov ebp,esp
6A8E6B68 push ecx
6A8E6B69 push esi
6A8E6B6A mov esi,ecx
if (this->bbWeight == BB_ZERO_WEIGHT)
6A8E6B6C movss xmm0,dword ptr [esi+20h]
6A8E6B71 ucomiss xmm0,dword ptr [__real@00000000 (6AA18AB4h)]
6A8E6B78 lahf
6A8E6B79 test ah,44h
6A8E6B7C jp BasicBlock::getBBWeight+1Dh (6A8E6B82h)
{
return BB_ZERO_WEIGHT;
6A8E6B7E fldz
6A8E6B80 jmp BasicBlock::getBBWeight+34h (6A8E6B99h)
}
else
{
weight_t calledCount = getCalledCount(comp);
6A8E6B82 push dword ptr [comp]
6A8E6B85 call BasicBlock::getCalledCount (6A8E6B9Eh)
6A8E6B8A fstp dword ptr [ebp-4]
// Normalize the bbWeights by multiplying by BB_UNITY_WEIGHT and dividing by the calledCount.
//
weight_t fullResult = this->bbWeight * BB_UNITY_WEIGHT / calledCount;
6A8E6B8D fld dword ptr [esi+20h]
6A8E6B90 fmul dword ptr [__real@42c80000 (6AA1600Ch)]
6A8E6B96 fdiv dword ptr [calledCount]
6A8E6B99 pop esi
return fullResult;
}
}
6A8E6B9A leave
6A8E6B9B ret 4 while the generated code in release is // getBBWeight -- get the normalized weight of this block
BasicBlock::weight_t BasicBlock::getBBWeight(Compiler* comp)
{
6A777140 push ebp
6A777141 mov ebp,esp
if (this->bbWeight == BB_ZERO_WEIGHT)
6A777143 movss xmm0,dword ptr [ecx+20h]
6A777148 xorps xmm2,xmm2
6A77714B ucomiss xmm0,xmm2
6A77714E lahf
6A77714F test ah,44h
6A777152 jp BasicBlock::getBBWeight+1Bh (6A77715Bh)
6A777154 xorps xmm0,xmm0
return fullResult;
}
}
6A777157 pop ebp
6A777158 ret 4
{
return BB_ZERO_WEIGHT;
}
else
{
weight_t calledCount = getCalledCount(comp);
6A77715B mov ecx,dword ptr [comp]
6A77715E movss xmm3,dword ptr [__real@42c80000 (6A8257DCh)]
6A777166 movss xmm1,dword ptr [ecx+164h]
6A77716E ucomiss xmm1,xmm2
6A777171 lahf
6A777172 test ah,44h
6A777175 jp BasicBlock::getBBWeight+7Ch (6A7771BCh)
6A777177 mov eax,dword ptr [ecx+3F8h]
6A77717D mov eax,dword ptr [eax]
6A77717F and eax,4000000h
6A777184 or eax,0
6A777187 jne BasicBlock::getBBWeight+65h (6A7771A5h)
6A777189 cmp dword ptr [ecx+1B4h],eax
6A77718F je BasicBlock::getBBWeight+65h (6A7771A5h)
6A777191 movss xmm1,dword ptr [__real@3f800000 (6A8255FCh)]
// Normalize the bbWeights by multiplying by BB_UNITY_WEIGHT and dividing by the calledCount.
//
weight_t fullResult = this->bbWeight * BB_UNITY_WEIGHT / calledCount;
6A777199 mulss xmm0,xmm3
6A77719D divss xmm0,xmm1
return fullResult;
}
}
6A7771A1 pop ebp
6A7771A2 ret 4
{
return BB_ZERO_WEIGHT;
}
else
{
weight_t calledCount = getCalledCount(comp);
6A7771A5 mov eax,dword ptr [ecx+100h]
6A7771AB movss xmm1,dword ptr [eax+20h]
6A7771B0 ucomiss xmm1,xmm2
6A7771B3 lahf
6A7771B4 test ah,44h
6A7771B7 jp BasicBlock::getBBWeight+7Ch (6A7771BCh)
6A7771B9 movaps xmm1,xmm3
// Normalize the bbWeights by multiplying by BB_UNITY_WEIGHT and dividing by the calledCount.
//
weight_t fullResult = this->bbWeight * BB_UNITY_WEIGHT / calledCount;
6A7771BC mulss xmm0,xmm3
6A7771C0 divss xmm0,xmm1
return fullResult;
}
}
6A7771C4 pop ebp
6A7771C5 ret 4 |
|
The difference seems to be when whole program optimization is enabled, in which case MSVC gives Not sure if we can enable whole program optimization in checked builds. Might be that we need to introduce the epsilon above, though I worry that we can see other differences. |
|
The use of floating-point in the compiler was always going to be a risk due to issues like this, but we do need some kind of solution. What FP switch is used for VC++? Presumably This also implies we should verify no checked/release diffs on Linux/Mac, which will have different native compiler behavior. Tangentially related: #53849 |
|
We are using |
|
The docs say, "The compiler rounds to source code precision at four specific points during expression evaluation: ... when a function call returns a floating-point value." Is that happening in the checked case? The calculation occurs and result is left on the stack; does it need an explicit rounding? Would that even matter to the issue? |
|
Could be if we revised everything to use |
|
The issue boils down to the fact that 99.429672f * 100 / 100gives different results depending on whether it is evaluated with 64-bit or 32-bit intermediate precision. In debug and release MSVC is evaluating it with 32-bit intermediate precision, while checked builds without LTO evaluate it with 64-bit (or 80-bit) intermediate precision. I read various sources that suggest that MSVC will set the intermediate precision of x87 instructions to 64-bit, so indeed switching to double might fix that. Although since the JIT is a library, I guess there is no guarantee that the intermediate precision is not 80 bits in which case we might just hit it less often, as @AndyAyersMS mentioned. |
|
Would adding explicit casts to this particular code help? (e.g., That's not a great solution compiler-wide given pervasive computations using |
It should, but as you note it may be hard to do this everywhere it's needed. Another trick is to struct-wrap But I think going to |
|
Adding the cast does not seem to affect the code gen -- the problem remains. |
|
I verified that switching to double fixes the problem and also the diffs in the .mch files @AndyAyersMS sent above. I'll submit a PR shortly. |
|
Since there is no codegen bug here I'll move it to 7.0. |
|
Unfortunately with using doubles everywhere (including RA) I see a ~1.2% regression on x64 in instructions retired with PIN of SPMI over libraries. That's with the following changeset (only last commit is important): |
|
Andy suggested that the regression might be because of native PGO and after rebuilding a release build with PGO off I see almost no difference (0.03% more retired instructions), so gonna mitigate by making the weights doubles and also by introducing epsilon in the above. |
Mitigate issues where the compiler might evaluate intermediate computations with higher precisions due to optimizations, causing different results between x86 windows checked/release. Fix dotnet#58063
ghost
added
the
in-pr
ghost
removed
the
in-pr
ghost
locked as resolved and limited conversation to collaborators
Seven methods show Check-Release diffs with x86 (at b8a5a4a, jit guid c2e4a466-795d-4e64-a776-0ae7b2eed65b)
In windows.x86\libraries.pmi.windows.x86.checked.mch
In windows.x86\libraries_tests.pmi.windows.x86.checked.mch
Sample diff (322112)
Sample repro:
The text was updated successfully, but these errors were encountered: