Revert removal of SuppressGCTransition from SystemNative_GetTimestamp() #27473
Conversation
… enclosing the unmanaged call.
|
/cc @jkotas @dotnet/jit-contrib |
There was a problem hiding this comment.
The change looks reasonable.
How urgent is it? Do we need to merge it ASAP?
Could you please show the jitdump before and after your change? As I understand the additional statement is inserted earlier in the block now. We will need asm diffs for that because it could change later optimizations behavior.
It would be good to have your windows repro in coreclr\tests\src\JIT\Regression\JitBlue\GitHub_27465.
No rush, can wait until next week.
I can, but note this is only updating function calls that I previously changed in in #26458. Also, what is the metric for jit-diff? I don't always see it provided with codegen changes and am having issue trying to understand when it is required and when it isn't.
I don't agree. This was discovered via an assert and fired because of usage of bad legacy code, not a regression from a known good state. Further the example doesn't capture the actual issue which is related to an assert related to tail call validation and insertion of calls at the end of a basic block. Neither of which have anything to do with the usage of the |
Hm, that is not easy to express. It is a good habbit to have them always collected, sometimes we do, but don't mention it in simple PRs (if the diffs are zero). In this particular case I think it will be useful to see:
I expect to see diffs but the total size should be the same (so only code movements). If we see a size regression then we will need to check |
|
Hitting an issue with my change to inserting a new statement: Line 14859 in b46c857 if (lastStmt->GetRootNode()->gtOper == GT_CALL)
{
noway_assert(fgRemoveRestOfBlock);
...The code to reproduce this is below. It looks like the new statement is causing issues during condition folding - see stack below example. using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Threading;
using System.Runtime.InteropServices;
namespace System.Runtime.InteropServices
{
public class SuppressGCTransitionAttribute : Attribute
{
public SuppressGCTransitionAttribute() { }
}
}
static class NativeLib
{
public const string Path = @"NativeLib.dll";
[DllImport(Path, EntryPoint = "SystemNative_GetTimestamp")]
[SuppressGCTransition]
internal static extern ulong GetTimestamp();
}
public unsafe class Program
{
private static long TS => (long)NativeLib.GetTimestamp();
static void Main(string[] args)
{
if (RunTest(TS))
throw new Exception();
}
static bool RunTest(long tt)
{
long ts1 = TS;
if (ts1 == tt)
{
return true;
}
return false;
}
} |
Review feedback
Codegen regression tests are "interesting sequences" that happened to trigger a bug. They rarely target specific obvious invariants. |
@sandreenko I am rethinking my statement here. I am discovering a lot of nuance with this work and am leaning more toward it being a good idea. Instead of putting this under the regression folder is there a better place for targeted scenario tests? I am thinking of one related to insertion of GC_POLLs. I have a few scenarios that we should validate. |
|
@jkotas Agreed. This feature is becoming far more complex and I am writing a few examples that I have discovered that will need to be handled by any work improving the insert |
The GC_POLL is more effective after the the unmanaged call. (It covers more time where the GC could not have been suspended after the call.) It would be nice to keep it inserting after the call. |
@jkotas I don't understand why that would be the case. In tight loops regardless of which side of the call it will be hit and if unmanaged calls are stacked then we cascade the polling. If we really want this after the call in a robust manner, then I am going to have to defer this to the JIT team since there are significant issues in this area that I don't quite know how to address. To unpack this a bit. It is going to require reordering statements and creating additional basic blocks. For example, |
I think https://github.com/dotnet/coreclr/tree/master/tests/src/JIT/Methodical is the best match for it.
I can take a look at the assert in morph, but only tomorrow. |
|
I believe the solution here is to move Welcome to other suggestions if the above doesn't make sense. |
|
After speaking with various stakeholders, I am going to keep the changes I have and provide |
|
@dotnet/jit-contrib |
|
This change seems reasonable to me, but wrt the comment above about possibly moving the insertion to the |
|
@CarolEidt Thanks for the additional context. I am going to defer that potential option to the future, possibly with work done to address https://github.com/dotnet/coreclr/issues/27186. Speaking to @briansull, he was also unconvinced it should be done in the Rationalizer phase. |
|
@CarolEidt @sandreenko @jkotas Any additional feedback? |
There was a problem hiding this comment.
LGTM, thanks for the tests.
asm diffs are from different register allocations:
prolog before:
G_M40076_IG01: ; func=00, offs=000000H, size=000DH, bbWeight=1 , gcrefRegs=00000000 {}, byrefRegs=00000000 {}, byref, nogc <-- Prolog IG
IN0012: 000000 push rbp
IN0013: 000001 push rdi
IN0014: 000002 push rsi
IN0015: 000003 push rbx <- used to save rdi GC pointer across the call to CORINFO_HELP_POLL_GC
IN0016: 000004 sub rsp, 40
IN0017: 000008 lea rbp, [rsp+40H]
prolog after (`rdi comes alive after the call now, so don't need to save it):
IN0011: 000000 push rbp
IN0012: 000001 push rdi
IN0013: 000002 push rsi
IN0014: 000003 sub rsp, 48
Instead of relying on the existing logic in
fgCreateGCPoll()that inserts aGC_POLLat the end of aBasicBlock, supply a statement contained in the block and insert a new statement with theGC_POLLafter the supplied statement.Fixes https://github.com/dotnet/coreclr/issues/27465