search

dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.97k stars 4.65k forks source link

JIT: impDevirtualizeCall might discard commas #67982

Open jakobbotsch opened 2 years ago

jakobbotsch commented 2 years ago

impDevirtualizeCall uses gtEffectiveVal when accessing 'this', indicating that we may see commas here: https://github.com/dotnet/runtime/blob/3b6a539c7219383ec6181a112465f904fe9e2edb/src/coreclr/jit/importer.cpp#L21094 Later in the function we do an optimization for boxes where we replace 'this' without taking commas into account: https://github.com/dotnet/runtime/blob/3b6a539c7219383ec6181a112465f904fe9e2edb/src/coreclr/jit/importer.cpp#L21456 https://github.com/dotnet/runtime/blob/3b6a539c7219383ec6181a112465f904fe9e2edb/src/coreclr/jit/importer.cpp#L21517

67238 adds an assert that we didn't actually have a comma here, but this might not be right.

category:correctness theme:devirtualization skill-level:beginner cost:small impact:small

ghost commented 2 years ago

Tagging subscribers to this area: @JulieLeeMSFT See info in area-owners.md if you want to be subscribed.

Issue Details
`impDevirtualizeCall` uses `gtEffectiveVal` when accessing 'this', indicating that we may see commas here: https://github.com/dotnet/runtime/blob/3b6a539c7219383ec6181a112465f904fe9e2edb/src/coreclr/jit/importer.cpp#L21094 Later in the function we do an optimization for boxes where we replace 'this' without taking commas into account: https://github.com/dotnet/runtime/blob/3b6a539c7219383ec6181a112465f904fe9e2edb/src/coreclr/jit/importer.cpp#L21456 https://github.com/dotnet/runtime/blob/3b6a539c7219383ec6181a112465f904fe9e2edb/src/coreclr/jit/importer.cpp#L21517 #67238 adds an assert that we didn't actually have a comma here, but this might not be right.
Author: jakobbotsch
Assignees: -
Labels: `area-CodeGen-coreclr`, `untriaged`
Milestone: -
AndyAyersMS commented 2 years ago

Tried coming up with a case where we'd have a GT_BOX in a comma but couldn't find one. The importer doesn't create that many commas, and the ones it creates are for stylized patterns. Not saying it can never happen.

A more likely possibility is that we run into one of these during late devirtualization, after more phases have run.

AndyAyersMS commented 2 years ago

I haven't been able to create a case where this happens, so will move this out of 7.0.