JIT: Wrong result computed with forward sub enabled on x64 Windows/Linux #68049

jakobbotsch · 2022-04-14T20:31:52Z

Description

The following program computes a wrong result when forward sub is enabled.

Reproduction Steps

// Generated by Fuzzlyn v1.5 on 2022-04-13 11:38:00
// Run on X64 Linux
// Seed: 1784259920377383051
// Reduced from 110.5 KiB to 0.9 KiB in 00:00:57
// Debug: Outputs 1
// Release: Outputs 0
public struct S0
{
    public uint F0;
    public long F1;
    public S0(uint f0): this()
    {
        F0 = f0;
    }

    public ulong M5()
    {
        var vr1 = new ushort[]{0};
        M6(vr1);
        return 1;
    }

    public void M6(ushort[] arg0)
    {
        this = new S0(0);
    }
}

public class Program
{
    public static IRuntime s_rt;
    public static void Main()
    {
        s_rt = new Runtime();
        var vr4 = new S0[]{new S0(1)};
        var vr5 = new short[]{0};
        bool vr6 = M1(vr4, vr5) <= 1;
    }

    public static short M1(S0[] arg0, short[] arg1)
    {
        long var3 = arg0[0].F1;
        var3 = (arg0[0].F0 & (byte)arg0[0].M5());
        s_rt.WriteLine(var3);
        return arg1[0];
    }
}

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

public class Runtime : IRuntime
{
    public void WriteLine<T>(T value) => System.Console.WriteLine(value);
}

Expected behavior

Same result.

Actual behavior

Different result.

Regression?

No response

Known Workarounds

No response

Configuration

Reproduces on both x64 Windows and Linux.

Other information

cc @dotnet/jit-contrib

ghost · 2022-04-14T20:50:29Z

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

Issue Details

Description

The following program computes a wrong result when forward sub is enabled.

Reproduction Steps

// Generated by Fuzzlyn v1.5 on 2022-04-13 11:38:00
// Run on X64 Linux
// Seed: 1784259920377383051
// Reduced from 110.5 KiB to 0.9 KiB in 00:00:57
// Debug: Outputs 1
// Release: Outputs 0
public struct S0
{
    public uint F0;
    public long F1;
    public S0(uint f0): this()
    {
        F0 = f0;
    }

    public ulong M5()
    {
        var vr1 = new ushort[]{0};
        M6(vr1);
        return 1;
    }

    public void M6(ushort[] arg0)
    {
        this = new S0(0);
    }
}

public class Program
{
    public static IRuntime s_rt;
    public static void Main()
    {
        s_rt = new Runtime();
        var vr4 = new S0[]{new S0(1)};
        var vr5 = new short[]{0};
        bool vr6 = M1(vr4, vr5) <= 1;
    }

    public static short M1(S0[] arg0, short[] arg1)
    {
        long var3 = arg0[0].F1;
        var3 = (arg0[0].F0 & (byte)arg0[0].M5());
        s_rt.WriteLine(var3);
        return arg1[0];
    }
}

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

public class Runtime : IRuntime
{
    public void WriteLine<T>(T value) => System.Console.WriteLine(value);
}

Expected behavior

Same result.

Actual behavior

Different result.

Regression?

No response

Known Workarounds

No response

Configuration

Reproduces on both x64 Windows and Linux.

Other information

cc @dotnet/jit-contrib

Author:	jakobbotsch
Assignees:	-
Labels:	`area-CodeGen-coreclr`, `untriaged`
Milestone:	-

jakobbotsch · 2022-04-14T21:22:23Z

Similar looking example:

// Generated by Fuzzlyn v1.5 on 2022-04-13 11:57:44
// Run on X64 Windows
// Seed: 15524766127402705757
// Reduced from 138.3 KiB to 1.6 KiB in 00:02:03
// Debug: Outputs 1
// Release: Outputs 254
public struct S0
{
    public short F0;
    public int F3;
    public byte M29(ref sbyte[] arg0)
    {
        if (this.F0 > this.F3)
        {
            this.F0 = (short)~this.F0;
            sbyte[, ] var0 = new sbyte[, ]{{1}};
        }

        return Program.M31();
    }
}

public class Program
{
    public static IRuntime s_rt;
    public static short[] s_2 = new short[]{0};
    public static sbyte[][] s_8 = new sbyte[][]{new sbyte[]{1}};
    public static byte s_9 = 28;
    public static void Main()
    {
        CollectibleALC alc = new CollectibleALC();
        System.Reflection.Assembly asm = alc.LoadFromAssemblyPath(System.Reflection.Assembly.GetExecutingAssembly().Location);
        System.Reflection.MethodInfo mi = asm.GetType(typeof(Program).FullName).GetMethod(nameof(MainInner));
        System.Type runtimeTy = asm.GetType(typeof(Runtime).FullName);
        mi.Invoke(null, new object[]{System.Activator.CreateInstance(runtimeTy)});
    }

    public static void MainInner(IRuntime rt)
    {
        s_rt = rt;
        S0[] vr2 = new S0[]{new S0()};
        s_2[0] = vr2[0].F0++;
        byte vr3 = (byte)(vr2[0].F0 % (long)vr2[0].M29(ref s_8[0]));
        s_rt.WriteLine("c_747", vr3);
    }

    public static byte M31()
    {
        return s_9;
    }
}

public interface IRuntime
{
    void WriteLine<T>(string site, T value);
}

public class Runtime : IRuntime
{
    public void WriteLine<T>(string site, T value) => System.Console.WriteLine(value);
}

public class CollectibleALC : System.Runtime.Loader.AssemblyLoadContext
{
    public CollectibleALC(): base(true)
    {
    }
}

Not sure why these are just showing up now.

AndyAyersMS · 2022-04-14T21:51:38Z

Not sure why these are just showing up now.

Yeah, seems odd -- maybe once we root cause this it will make sense.

AndyAyersMS · 2022-04-15T01:38:37Z

In the second case, it looks like during assertion prop we decide we can reverse the order of the operands to the MOD, but this is unsafe. Tracking that down now.

***** BB01
STMT00009 ( ??? ... ??? )
N038 ( 77, 54) [000073] -ACXG---R---              *  ASG       int    $1e7
N037 (  3,  2) [000072] D------N----              +--*  LCL_VAR   int    V02 loc1         d:2 $VN.Void
N036 ( 73, 51) [000071] -ACXG-------              \--*  CAST      int <- ubyte <- long <l:$48e, c:$490>
N035 ( 72, 49) [000070] -ACXG-------                 \--*  MOD       long   <l:$586, c:$585>
N008 (  6,  7) [000050] ----G-------                    +--*  CAST      long <- int <l:$580, c:$581>
N007 (  5,  5) [000135] ----G--N----                    |  \--*  COMMA     short  <l:$48c, c:$441>

...

VN based non-null prop in BB01:
N020 (  4,  4) [000060] ---XG-------              *  IND       ref    <l:$1e1, c:$1e0>
ReMorphing args for 64.CALL:
ReMorphing args for 63.CALL:
argSlots=3, preallocatedArgCount=4, nextSlotNum=4, nextSlotByteOffset=32, outgoingArgSpaceSize=32
ArgTable for 63.CALL after fgMorphArgs:
fgArgTabEntry[arg 0 152.LCL_VAR ref (By ref), 1 reg: rcx, byteAlignment=8, lateArgInx=0, tmpNum=V10, isTmp, processed]
fgArgTabEntry[arg 1 61.CNS_INT int (By ref), 1 reg: rdx, byteAlignment=8, lateArgInx=1, processed]
fgArgTabEntry[arg 2 62.CNS_INT long (By ref), 1 reg: r8, byteAlignment=8, lateArgInx=2, processed]

argSlots=2, preallocatedArgCount=4, nextSlotNum=4, nextSlotByteOffset=32, outgoingArgSpaceSize=32
ArgTable for 64.CALL after fgMorphArgs:
fgArgTabEntry[arg 1 157.LCL_VAR byref (By ref), 1 reg: rdx, byteAlignment=8, lateArgInx=0, tmpNum=V11, isTmp, processed]
fgArgTabEntry[arg 0 160.LCL_VAR byref (By ref), 1 reg: rcx, byteAlignment=8, lateArgInx=1, tmpNum=V12, isTmp, processed]

optVNAssertionPropCurStmt morphed tree:
N034 ( 77, 54) [000073] -ACXGO--R---              *  ASG       int    $1e7
N033 (  3,  2) [000072] D------N----              +--*  LCL_VAR   int    V02 loc1         d:2 $VN.Void
N032 ( 73, 51) [000071] -ACXGO------              \--*  CAST      int <- ubyte <- long <l:$48e, c:$490>
N031 ( 72, 49) [000070] -ACXGO--R---                 \--*  MOD       long   <l:$586, c:$585>
N030 (  6,  7) [000050] ------------                    +--*  CAST      long <- int <l:$580, c:$581>
N029 (  5,  5) [000136] *------N----                    |  \--*  IND       short  <l:$48c, c:$441>

Note the R flag on [000070].

AndyAyersMS · 2022-04-15T01:56:00Z

Ah, looks like the culprit setting R is gtSetEvalOrder because gtCanSwap applied to the mod operands returns true.

Note in the above [000050] has lost it's G flag somehow and so doesn't appear to interfere with the other mod operand, which is

N024 ( 46, 39) [000069] -ACXGO---U--                    \--*  CAST      long <- ulong <- uint $582

so the bug is that we lose flags on the first operand subtree.

Before remorphing in VN:

N008 (  6,  7) [000050] ----G-------                    +--*  CAST      long <- int <l:$580, c:$581>
N007 (  5,  5) [000135] ----G--N----                    |  \--*  COMMA     short  <l:$48c, c:$441>
N001 (  0,  0) [000128] ------------                    |     +--*  NOP       void   $301
N006 (  5,  5) [000136] *-----------                    |     \--*  IND       short  <l:$48c, c:$441>
N005 (  1,  2) [000134] -------N----                    |        \--*  ARR_ADDR  byref S0[] Zero Fseq[F0] $340
N004 (  1,  2) [000133] -------N----                    |           \--*  ADD       byref  $204
N002 (  1,  1) [000125] ------------                    |              +--*  LCL_VAR   ref    V01 loc0         u:2 $2c0
N003 (  1,  1) [000132] ------------                    |              \--*  CNS_INT   long   16 $104

after remorphing

N030 (  6,  7) [000050] ------------                    +--*  CAST      long <- int <l:$580, c:$581>
N029 (  5,  5) [000136] *------N----                    |  \--*  IND       short  <l:$48c, c:$441>
N028 (  1,  2) [000134] -------N----                    |     \--*  ARR_ADDR  byref S0[] Zero Fseq[F0] $340
N027 (  1,  2) [000133] -------N----                    |        \--*  ADD       byref  $204
N025 (  1,  1) [000125] ------------                    |           +--*  LCL_VAR   ref    V01 loc0         u:2 $2c0
N026 (  1,  1) [000132] ------------                    |           \--*  CNS_INT   long   16 $104

and this appears to be caused by [000136] not having a G flag despite being a global ref, so remorphing clears G from ancestor nodes. And [000136] is created during morph from [000047]:

fgMorphArrayIndex (after remorph):
               [000135] ---XG+------              *  COMMA     struct
               [000128] ---X-+------              +--*  BOUNDS_CHECK_Rng void  
               [000046] -----+------              |  +--*  CNS_INT   int    0
               [000127] ---X-+------              |  \--*  ARR_LENGTH int   
               [000045] -----+------              |     \--*  LCL_VAR   ref    V01 loc0         
               [000047] n---G+-N----              \--*  IND       struct
               [000134] -----+-N----                 \--*  ARR_ADDR  byref S0[]
               [000133] -----+------                    \--*  ADD       byref 
               [000125] -----+------                       +--*  LCL_VAR   ref    V01 loc0         
               [000132] -----+------                       \--*  CNS_INT   long   16

fgAddFieldSeqForZeroOffset for Fseq[F0]
addr (Before)
               [000134] -----+-N----                ARR_ADDR  byref S0[]
     (After)
               [000134] -----+-N----                ARR_ADDR  byref S0[] Zero Fseq[F0]

Final value of Compiler::fgMorphField after calling fgMorphSmpOp:
               [000135] ---XG+------              *  COMMA     short 
               [000128] ---X-+------              +--*  BOUNDS_CHECK_Rng void  
               [000046] -----+------              |  +--*  CNS_INT   int    0
               [000127] ---X-+------              |  \--*  ARR_LENGTH int   
               [000045] -----+------              |     \--*  LCL_VAR   ref    V01 loc0         
               [000136] *----+------              \--*  IND       short 
               [000134] -----+-N----                 \--*  ARR_ADDR  byref S0[] Zero Fseq[F0]
               [000133] -----+------                    \--*  ADD       byref 
               [000125] -----+------                       +--*  LCL_VAR   ref    V01 loc0         
               [000132] -----+------                       \--*  CNS_INT   long   16

So, need to see why morph is losing this flag.

AndyAyersMS · 2022-04-15T02:11:59Z

This seems to be the culprit:

runtime/src/coreclr/jit/morph.cpp

Lines 12958 to 12996 in 0b1a80d

    
                       // Only do this optimization when we are in the global optimizer. Doing this after value numbering 
        
                       // could result in an invalid value number for the newly generated GT_IND node. 
        
                       if ((op1->OperGet() == GT_COMMA) && fgGlobalMorph) 
        
                       { 
        
                           // Perform the transform IND(COMMA(x, ..., z)) == COMMA(x, ..., IND(z)). 
        
                           // TBD: this transformation is currently necessary for correctness -- it might 
        
                           // be good to analyze the failures that result if we don't do this, and fix them 
        
                           // in other ways.  Ideally, this should be optional. 
        
                           GenTree*     commaNode = op1; 
        
                           GenTreeFlags treeFlags = tree->gtFlags; 
        
                           commaNode->gtType      = typ; 
        
                           commaNode->gtFlags     = (treeFlags & ~GTF_REVERSE_OPS); // Bashing the GT_COMMA flags here is 
        
                                                                                    // dangerous, clear the GTF_REVERSE_OPS at 
        
                                                                                    // least. 
        
           #ifdef DEBUG 
        
                           commaNode->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED; 
        
           #endif 
        
                           while (commaNode->AsOp()->gtOp2->gtOper == GT_COMMA) 
        
                           { 
        
                               commaNode         = commaNode->AsOp()->gtOp2; 
        
                               commaNode->gtType = typ; 
        
                               commaNode->gtFlags = 
        
                                   (treeFlags & ~GTF_REVERSE_OPS & ~GTF_ASG & ~GTF_CALL); // Bashing the GT_COMMA flags here is 
        
                               // dangerous, clear the GTF_REVERSE_OPS, GT_ASG, and GT_CALL at 
        
                               // least. 
        
                               commaNode->gtFlags |= ((commaNode->AsOp()->gtOp1->gtFlags | commaNode->AsOp()->gtOp2->gtFlags) & 
        
                                                      (GTF_ASG | GTF_CALL)); 
        
           #ifdef DEBUG 
        
                               commaNode->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED; 
        
           #endif 
        
                           } 
        
                           tree          = op1; 
        
                           GenTree* addr = commaNode->AsOp()->gtOp2; 
        
                           // TODO-1stClassStructs: we often create a struct IND without a handle, fix it. 
        
                           op1 = gtNewIndir(typ, addr); 
        
                           // This is very conservative 
        
                           op1->gtFlags |= treeFlags & ~GTF_ALL_EFFECT & ~GTF_IND_NONFAULTING; 
        
                           op1->gtFlags |= (addr->gtFlags & GTF_ALL_EFFECT);

AndyAyersMS · 2022-04-15T02:40:03Z

Worked up a plausible fix. Testing it locally.

AndyAyersMS · 2022-04-15T03:08:25Z

Seems to be holding up: main...AndyAyersMS:Fix68049

Minimal SPMI diffs. Will put up a PR once I've set up test cases.

jakobbotsch · 2022-04-15T08:14:13Z

Perhaps exposed by #64581?

SingleAccretion · 2022-04-15T14:18:38Z

Perhaps exposed by #64581?

Definitely. We used to have two INDs there, presumably one of them held onto the GLOB_REF.

An alternative fix would be to copy the GLOB_REF from the original indirection (i. e. from treeFlags).

(Note that fgIsIndirOfAddrOfLocal is not sufficient to omit GLOB_REF, it must be fgIsIndirOfAddrOfLocal() && !lcl->IsAddressExposed())

Morph may transform `IND(COMMA(..., z))` into `COMMA(... , IND(z))` and when doing so it was not computing appropriately conservative flags for the new `IND`. In particular it was losing `GTF_GLOB_REF`. This allowed subsequent opts to reorder operands in an unsafe manner. Fixes dotnet#68049.

AndyAyersMS · 2022-04-15T17:53:53Z

An alternative fix would be to copy the GLOB_REF from the original indirection (i. e. from treeFlags).

Right. Went with something like this.

There are a few other places in morph with similar "idoms" for setting indir flags. Perhaps what they're doing is ok. Hard to tell.

Seems like we might want to set up a stress mode that reverses operands when we think it's legal to do so, regardless of whether it is "cost effective". Or perhaps a randomized costing mode that really tries to scramble things up.

Morph may transform `IND(COMMA(..., z))` into `COMMA(... , IND(z))` and when doing so it was not computing appropriately conservative flags for the new `IND`. In particular it was losing `GTF_GLOB_REF`. This allowed subsequent opts to reorder operands in an unsafe manner. Fixes #68049.

dotnet-issue-labeler bot added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI untriaged New issue has not been triaged by the area owner labels Apr 14, 2022

AndyAyersMS self-assigned this Apr 15, 2022

AndyAyersMS removed the untriaged New issue has not been triaged by the area owner label Apr 15, 2022

AndyAyersMS mentioned this issue Apr 15, 2022

JIT: fix flags lossage in morph comma/ind interchange #68082

Merged

ghost added the in-pr There is an active PR which will close this issue when it is merged label Apr 15, 2022

AndyAyersMS closed this as completed in #68082 Apr 16, 2022

ghost removed the in-pr There is an active PR which will close this issue when it is merged label Apr 16, 2022

ghost locked as resolved and limited conversation to collaborators May 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JIT: Wrong result computed with forward sub enabled on x64 Windows/Linux #68049

JIT: Wrong result computed with forward sub enabled on x64 Windows/Linux #68049

jakobbotsch commented Apr 14, 2022

ghost commented Apr 14, 2022

Description

Reproduction Steps

Expected behavior

Actual behavior

Regression?

Known Workarounds

Configuration

Other information

jakobbotsch commented Apr 14, 2022

AndyAyersMS commented Apr 14, 2022

AndyAyersMS commented Apr 15, 2022

AndyAyersMS commented Apr 15, 2022

AndyAyersMS commented Apr 15, 2022

AndyAyersMS commented Apr 15, 2022

AndyAyersMS commented Apr 15, 2022

jakobbotsch commented Apr 15, 2022

SingleAccretion commented Apr 15, 2022

AndyAyersMS commented Apr 15, 2022

JIT: Wrong result computed with forward sub enabled on x64 Windows/Linux #68049

JIT: Wrong result computed with forward sub enabled on x64 Windows/Linux #68049

Comments

jakobbotsch commented Apr 14, 2022

Description

Reproduction Steps

Expected behavior

Actual behavior

Regression?

Known Workarounds

Configuration

Other information

ghost commented Apr 14, 2022

Description

Reproduction Steps

Expected behavior

Actual behavior

Regression?

Known Workarounds

Configuration

Other information

jakobbotsch commented Apr 14, 2022

AndyAyersMS commented Apr 14, 2022

AndyAyersMS commented Apr 15, 2022

AndyAyersMS commented Apr 15, 2022

AndyAyersMS commented Apr 15, 2022

AndyAyersMS commented Apr 15, 2022

AndyAyersMS commented Apr 15, 2022

jakobbotsch commented Apr 15, 2022

SingleAccretion commented Apr 15, 2022

AndyAyersMS commented Apr 15, 2022