Skip to content

Conversation

@AaronRobinsonMSFT
Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT commented Apr 23, 2024

@ghost ghost added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Apr 23, 2024
@AaronRobinsonMSFT AaronRobinsonMSFT added test-enhancement Improvements of test source code area-TypeSystem-coreclr and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Apr 24, 2024
@AaronRobinsonMSFT AaronRobinsonMSFT added this to the 9.0.0 milestone Apr 24, 2024
@AlekseyTs
Copy link
Contributor

I am also curious if the following going to be considered a valid IL for Test1 method:

{
  // Code size       41 (0x29)
  .maxstack  1
  .locals init (T V_0) //t
  IL_0000:  ldarg.0
  IL_0001:  isinst     ""T""
  IL_0006:  brfalse.s  IL_0022
  IL_0008:  ldarg.0
  IL_0009:  isinst     ""T""
  IL_000e:  unbox.any  ""T""
  IL_0013:  stloc.0
  IL_0014:  ldloca.s   V_0
  IL_0016:  constrained. ""T""
  IL_001c:  callvirt   ""void I1.M()""
  IL_0021:  ret
  IL_0022:  ldc.i4.2
  IL_0023:  call       ""void System.Console.Write(int)""
  IL_0028:  ret
}
class Helper1<T>
    where T : I1, allows ref struct
{
    public static void Test1(I1 h1)
    {
        if (h1 is T t)
        {
            t.M();
        }
        else
        {
            System.Console.Write(2);
        }
    }
}
ref struct S : I1
{
    public void M()
    {
        System.Console.Write(3);
    }
}

interface I1
{
    void M();
}

class Program : I1
{
    static void Main()
    {
        Helper1<S>.Test1(new Program());
        Helper1<Program>.Test1(new Program());
    }

    public void M()
    {
        System.Console.Write(1);
    }
}

The behavior of IL is reasonable, but sequences isinst ; unbox.any and isinst ; br_true/false aren't explicitly listed as supported. However, box ; isinst ; unbox.any and box ; isinst ; br_true/false are listed as supported at https://github.com/dotnet/runtime/blob/main/docs/design/features/byreflike-generics.md#special-il-sequences.

This is not an ask from compiler to support an IL like that.

@AaronRobinsonMSFT
Copy link
Member Author

AaronRobinsonMSFT commented Apr 24, 2024

I am also curious if the following going to be considered a valid IL for Test1 method:

I will validate those sequences today.

The behavior of IL is reasonable, but sequences isinst ; unbox.any and isinst ; br_true/false aren't explicitly listed as supported.

@AlekseyTs Correct. The JIT only recognizes sequences in this area that begin with CEE_BOX. It is entirely possible those sequences work, but they are not special cased. See

//------------------------------------------------------------------------
// impBoxPatternMatch: match and import common box idioms
//
// Arguments:
// pResolvedToken - resolved token from the box operation
// codeAddr - position in IL stream after the box instruction
// codeEndp - end of IL stream
// opts - dictate pattern matching behavior
//
// Return Value:
// Number of IL bytes matched and imported, -1 otherwise
//
// Notes:
// pResolvedToken is known to be a value type; ref type boxing
// is handled in the CEE_BOX clause.
int Compiler::impBoxPatternMatch(CORINFO_RESOLVED_TOKEN* pResolvedToken,
const BYTE* codeAddr,
const BYTE* codeEndp,
BoxPatterns opts)

This is not an ask from compiler to support an IL like that.

Understood.

@fanyang-mono
Copy link
Member

I will take a look at the Mono failures.

@fanyang-mono
Copy link
Member

#101509 should fix the current mono test failures.

@AaronRobinsonMSFT
Copy link
Member Author

I am also curious if the following going to be considered a valid IL for Test1 method:

@AlekseyTs Yes, that is valid. I've also added a test for that case.

@AaronRobinsonMSFT
Copy link
Member Author

@lambdageek @fanyang-mono @steveisok Please take a look. The CI is now green.

Copy link
Member

@fanyang-mono fanyang-mono left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@AaronRobinsonMSFT AaronRobinsonMSFT merged commit f78c367 into dotnet:main Apr 25, 2024
@AaronRobinsonMSFT AaronRobinsonMSFT deleted the tests_byreflike_box_to_other branch April 25, 2024 18:16
matouskozak pushed a commit to matouskozak/runtime that referenced this pull request Apr 30, 2024
* Check if type is compatible right before emitting box

* Add test for using byreflike type in isinst expressions.
michaelgsharp pushed a commit to michaelgsharp/runtime that referenced this pull request May 9, 2024
* Check if type is compatible right before emitting box

* Add test for using byreflike type in isinst expressions.
@github-actions github-actions bot locked and limited conversation to collaborators May 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-TypeSystem-coreclr test-enhancement Improvements of test source code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants