Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add test for boxing byreflike fail. #101458

Merged

Conversation

AaronRobinsonMSFT
Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT commented Apr 23, 2024

@dotnet-issue-labeler dotnet-issue-labeler bot 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
69 checks passed
@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