Repository navigation
[JIT] Redundant Span<> bounds check not elided for fields in ref structs #10645
Description
From my review of various issues around bounds checks, the length check here should be sufficient to elide the span's bounds check:
private readonly ReadOnlySpan<int> _colStarts;
public int? TryParseInt(int col)
{
if ((uint)col < (uint)_colStarts.Length)
{
var parseSpan = _span.Slice(_colStarts[col]);
Utf8Parser.TryParse(parseSpan, out int result, out _);
return result;
}
return null;
}(Full code available here: https://gist.github.com/Zhentar/6464a6190c0007d83c95634d1a9889ed )
But it is still present in the jitted code, with the latest nightly (CoreCLR 4.6.26709.01, CoreFX 4.6.26606.05), 64bit RyuJIT
DelimitedFileParser.StrictFileParser+SpanLineParser.TryParseInt(Int32)
push rdi
push rsi
sub rsp,58h
mov rsi,rcx
lea rdi,[rsp+28h]
mov ecx,0Ch
xor eax,eax
rep stos dword ptr [rdi]
mov rcx,rsi
; if ((uint)col < (uint)_colStarts.Length)
; ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
cmp dword ptr [rcx],ecx
lea r8,[rcx+10h]
cmp edx,dword ptr [r8+8]
jae M04_L00
; var parseSpan = _span.Slice(_colStarts[col]);
; ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
mov r8,rcx
cmp dword ptr [r8],r8d
add r8,10h
cmp edx,dword ptr [r8+8]
jae M04_L02
mov r8,qword ptr [r8]
movsxd rdx,edx
mov edx,dword ptr [r8+rdx*4]
mov r8d,dword ptr [rcx+8]
cmp edx,r8d
ja M04_L01
mov rcx,qword ptr [rcx]
sub r8d,edx
movsxd rdx,edx
add rcx,rdx```
category:cq
theme:bounds-checks
skill-level:expert
cost:medium
impact:medium
Activity
mikedn commentedon Jul 9, 2018
Range check elimination is rather conservative when type fields are involved (due to thread safety issues), it works best with local variables. Try copying
_colStartto a local variable:Zhentar commentedon Jul 9, 2018
I can give it a try to see if that is the cause, but it is a field in a
ref readonlystruct so it would be safe for the JIT to assume it will not be changing without needing a copy.mikedn commentedon Jul 9, 2018
readonlyis not of much use sine it's pretty easy to workaround it, much like C++'sconst.ref structis more interesting, such struct live only on the stack so it would seem that they should not have thread safety issues. But I don't think thread safety is actually guaranteed, it's still possible to take the address of such a struct and somehow pass it to another thread. That's likely extremely rare but not impossible.Zhentar commentedon Jul 9, 2018
As I recall, part of the reasoning for making Span stack only was that the resulting implicit thread-safety would prevent struct tearing, so would it not also be okay to make that assumption for bounds check elimination? (And for that matter, does not the bounds check itself rely upon it?)
mikedn commentedon Jul 9, 2018
Right, I need to double check that but it's quite possible that any means that would allow concurrent access to a stack location involve unsafe code. In that case it would probably reasonably for the JIT to assume that ref structs are never modified by other threads.
Bounds check optimizations assume that fields can be modified by other threads and the JIT doesn't do anything special in the case of ref structs. It used to be the case that bounds check elimination also worked in some cases that involved fields due to CSE but there was a bug in this area and its fix likely made the optimization more conservative.
mikedn commentedon Jul 9, 2018
Yep, it's impossible for code that escapes a pointer to a stack location to another thread to be safe. Not because the ECMA spec says so but simply because it's impossible to guarantee that the other thread won't attempt to dereference the pointer after the method that owns the stack location has returned.
And ECMA does indeed prevent verifiability by placing restrictions such as requiring
stfldnot to be an unmanaged pointer.So it does look like the JIT could be less conservative when dealing with fields of
ref structs.Somewhat related to #9512.
[-][JIT] Redundant Span<> bounds check not elided immediately after length check[/-][+][JIT] Redundant Span<> bounds check not elided for fields in ref structs[/+]RussKeldorph commentedon Jul 10, 2018
@dotnet/jit-contrib @CarolEidt