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

Track local var ranges for "interfering writes" LIR check #64804

Merged

Conversation

jakobbotsch
Copy link
Member

For LIR we verify that we can really consider locals to be used at their
user by having a checker that looks for interfering stores to the same
locals. However, in some cases we may have "interfering"
GT_LCL_FLD/GT_STORE_LCL_FLD, in the sense that they work on the same
local but on a disjoint range of bytes. Add support to validate this.

This fixes #57919 which made the fuzzer jobs very noisy and made it easy
to miss actual new examples (e.g. #63720 was just merged even though
there were real examples found there).

Fix #57919

For LIR we verify that we can really consider locals to be used at their
user by having a checker that looks for interfering stores to the same
locals. However, in some cases we may have "interfering"
GT_LCL_FLD/GT_STORE_LCL_FLD, in the sense that they work on the same
local but on a disjoint range of bytes. Add support to validate this.

This fixes dotnet#57919 which made the fuzzer jobs very noisy and made it easy
to miss actual new examples (e.g. dotnet#63720 was just merged even though
there were real examples found there).

Fix dotnet#57919
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Feb 4, 2022
@ghost ghost assigned jakobbotsch Feb 4, 2022
@ghost
Copy link

ghost commented Feb 4, 2022

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

Issue Details

For LIR we verify that we can really consider locals to be used at their
user by having a checker that looks for interfering stores to the same
locals. However, in some cases we may have "interfering"
GT_LCL_FLD/GT_STORE_LCL_FLD, in the sense that they work on the same
local but on a disjoint range of bytes. Add support to validate this.

This fixes #57919 which made the fuzzer jobs very noisy and made it easy
to miss actual new examples (e.g. #63720 was just merged even though
there were real examples found there).

Fix #57919

Author: jakobbotsch
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@jakobbotsch
Copy link
Member Author

/azp run Fuzzlyn

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jakobbotsch
Copy link
Member Author

cc @dotnet/jit-contrib

Also @AndyAyersMS there are some presumably forward-sub related failures in the Fuzzlyn run from this PR: https://dev.azure.com/dnceng/public/_build/results?buildId=1591834&view=ms.vss-build-web.run-extensions-tab

@AndyAyersMS
Copy link
Member

cc @dotnet/jit-contrib

Also @AndyAyersMS there are some presumably forward-sub related failures in the Fuzzlyn run from this PR: https://dev.azure.com/dnceng/public/_build/results?buildId=1591834&view=ms.vss-build-web.run-extensions-tab

Thanks, will take a look.

@AndyAyersMS
Copy link
Member

AndyAyersMS commented Feb 4, 2022

The x64 windows 7242107031351932946 looks like an issue in rationalize or lower

Forward sub creates

               [000049] --CXG-------              *  JTRUE     void  
               [000048] --CXG-------              \--*  LT        int   
               [000038] ---XG-------                 +--*  FIELD     long   F0
               [000037] ------------                 |  \--*  LCL_VAR   ref    V00 loc0         
               [000047] --CXG-------                 \--*  CAST      long <- int
               [000046] --CXG-------                    \--*  AND       int   
               [000040] --CXG-------                       +--*  CALL      int    Program.M35
               [000039] ------------ arg0                  |  \--*  LCL_VAR   ref    V00 loc0         
               [000045] ------------                       \--*  CNS_INT   int    0

Eventually this gets optimized to

N011 ( 27, 17) [000049] --CXGO------              *  JTRUE     void   $VN.Void
N010 ( 25, 15) [000048] J-CXGO-N----              \--*  LT        int    <l:$44, c:$480>
N004 (  6,  5) [000038] n---GO------                 +--*  IND       long   <l:$145, c:$c5>
N003 (  4,  3) [000091] -------N----                 |  \--*  ADD       byref  $282
N001 (  3,  2) [000037] ------------                 |     +--*  LCL_VAR   ref    V00 loc0         u:2 $1c6
N002 (  1,  1) [000090] ------------                 |     \--*  CNS_INT   long   8 field offset Fseq[F0] $141
N009 ( 18,  9) [000097] --CXG-------                 \--*  COMMA     long  
N007 ( 17,  8) [000040] --CXG-------                    +--*  CALL      int    Program.M35 $401
N006 (  3,  2) [000039] ------------ arg0 in rcx        |  \--*  LCL_VAR   ref    V00 loc0         u:2 (last use) $1c6
N008 (  1,  1) [000096] ------------                    \--*  CNS_INT   long   0 $142

with the expectation that the FIELD/IND will evaluate before the call, but they get reordered.

N003 (  4,  3) [000091] -c----------        t91 = *  LEA(b+8)  byref 
                                                  /--*  t91    byref  
N004 (  6,  5) [000038] nc--GO------        t38 = *  IND       long   <l:$145, c:$c5>
N006 (  3,  2) [000039] ------------        t39 =    LCL_VAR   ref    V00 loc0         u:2 (last use) $1c6
                                                  /--*  t39    ref    
               [000116] ------------       t116 = *  PUTARG_REG ref    REG rcx
                                                  /--*  t116   ref    arg0 in rcx
N007 ( 17,  8) [000040] --CXG-------        t40 = *  CALL      int    Program.M35 $401
N008 (  1,  1) [000096] -c----------        t96 =    CNS_INT   long   0 $142
                                                  /--*  t38    long   
                                                  +--*  t96    long   
N010 ( 25, 15) [000048] J--XGO-N----              *  LT        void   <l:$44, c:$480>
N011 ( 27, 17) [000049] ---XGO------              *  JTRUE     void   $VN.Void

@jakobbotsch
Copy link
Member Author

with the expectation that the FIELD/IND will evaluate before the call, but they get reordered.

It seems like something that Lowering::IsSafeToContainMem should be handling (assuming the containment is the problem)?

@AndyAyersMS
Copy link
Member

Yeah, xarch's Lowering::ContainCheckCompare is missing a safety check:

@@ -4949,7 +4949,7 @@ void Lowering::ContainCheckCompare(GenTreeOp* cmp)
         // we can treat the MemoryOp as contained.
         if (op1Type == op2Type)
         {
-            if (IsContainableMemoryOp(op1))
+            if (IsContainableMemoryOp(op1) && IsSafeToContainMem(cmp, op1))
             {
                 MakeSrcContained(cmp, op1);
             }

@AndyAyersMS
Copy link
Member

Let me see if this fixes those other cases too.

@jakobbotsch jakobbotsch merged commit 558a6dc into dotnet:main Feb 4, 2022
@jakobbotsch jakobbotsch deleted the silence-contained-reads-assert branch February 4, 2022 18:00
@AndyAyersMS
Copy link
Member

I can't repro the first x86 issue (seed 5821979800164656837).

@jakobbotsch
Copy link
Member Author

I can't repro the first x86 issue (seed 5821979800164656837).

This repros (in both x64 and x86):

// Generated by Fuzzlyn v1.5 on 2022-02-04 12:51:20
// Run on X86 Windows
// Seed: 5821979800164656837
// Reduced from 60.0 KiB to 0.3 KiB in 00:01:44
// Debug: Outputs 0
// Release: Outputs -1
public class Program
{
    public static IRT s_rt;
    public static long s_4;
    public static void Main()
    {
        s_rt = new C();
        int vr3 = (int)(s_4 & ~M7());
        s_rt.WriteLine(vr3);
    }

    public static short M7()
    {
        ref long var1 = ref s_4;
        var1 = 9223372036854775807L;
        return 0;
    }
}


public interface IRT
{
    void WriteLine<T>(T value);
}

public class C : IRT
{
    public void WriteLine<T>(T value)
    {
        System.Console.WriteLine(value);
    }
}

@AndyAyersMS
Copy link
Member

Second x86 issue (9341650659269168733) is fixed by the change to Lowering::ContainCheckCompare.

@AndyAyersMS
Copy link
Member

This repros (in both x64 and x86):

Thanks. This isn't fixed by the change to Lowering::ContainCheckCompare.

@AndyAyersMS
Copy link
Member

Think the third x86 issue is also fixed, but still need to verify that it will fail w/o the fix.

@AndyAyersMS
Copy link
Member

I can't get 17864721711828342668 to fail with main, at least on x86.

@AndyAyersMS
Copy link
Member

This repros (in both x64 and x86):

Thanks. This isn't fixed by the change to Lowering::ContainCheckCompare.

Looks like a similar sort of bug, trying to pin down exactly where we are missing a check. Here the parent is HWINTRINSIC int AndNot and we contain a memory load and move it past a call.

Just skimming though lowerxarch, the pattern of first calling IsContainableMemoryOp and then needing (and forgetting) to call IsSafeToContainMem seems pretty widespread. Hopefully it's not as bad as it looks at first blush.

@AndyAyersMS
Copy link
Member

I suppose luckily these cases are (were) somewhat rare? No diffs in SPMI from my first bit of fix above.

@ghost ghost locked as resolved and limited conversation to collaborators Mar 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Assertion failed '!nodeInfo.IsLclVarWrite() || !unusedLclVarReads.Contains(nodeInfo.LclNum())'
2 participants