JIT: impDevirtualizeCall might discard commas

[jakobbotsch]

impDevirtualizeCall uses gtEffectiveVal when accessing 'this', indicating that we may see commas here:

runtime/src/coreclr/jit/importer.cpp

Line 21094 in 3b6a539

GenTree* thisObj = call->gtCallThisArg->GetNode()->gtEffectiveVal(false);

Later in the function we do an optimization for boxes where we replace 'this' without taking commas into account:

runtime/src/coreclr/jit/importer.cpp

Line 21456 in 3b6a539

call->gtCallThisArg = gtNewCallArgs(localCopyThis);

runtime/src/coreclr/jit/importer.cpp

Line 21517 in 3b6a539

call->gtCallThisArg = gtNewCallArgs(localCopyThis);

#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]

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:

runtime/src/coreclr/jit/importer.cpp

Line 21094 in 3b6a539

GenTree* thisObj = call->gtCallThisArg->GetNode()->gtEffectiveVal(false);

Later in the function we do an optimization for boxes where we replace 'this' without taking commas into account:

runtime/src/coreclr/jit/importer.cpp

Line 21456 in 3b6a539

call->gtCallThisArg = gtNewCallArgs(localCopyThis);

runtime/src/coreclr/jit/importer.cpp

Line 21517 in 3b6a539

call->gtCallThisArg = gtNewCallArgs(localCopyThis);

#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]

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]

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