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

AndyAyersMS · 2022-04-15T17:44:38Z

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.

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.

ghost · 2022-04-15T17:44:44Z

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

Issue Details

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.

Author:	AndyAyersMS
Assignees:	-
Labels:	`area-CodeGen-coreclr`
Milestone:	-

AndyAyersMS · 2022-04-15T17:45:27Z

@jakobbotsch PTAL
cc @dotnet/jit-contrib

Leads to a few diffs in SPMI.

SingleAccretion · 2022-04-15T18:16:56Z

src/coreclr/jit/morph.cpp

                op1->gtFlags |= (addr->gtFlags & GTF_ALL_EFFECT);
+                op1->gtFlags |= treeFlags & (GTF_GLOB_REF | GTF_IND_FLAGS);


Is the GTF_IND_FLAGS portion needed? Seems like it'd be set by the |= treeFlags & ~GTF_ALL_EFFECT; above.

Is the GTF_IND_FLAGS portion needed?

No, it's not needed.

jakobbotsch · 2022-04-15T19:20:39Z

src/tests/JIT/Regression/JitBlue/Runtime_68049/Runtime_68049_1.cs

+        CollectibleALC alc = new CollectibleALC();
+        System.Reflection.Assembly asm = alc.LoadFromAssemblyPath(System.Reflection.Assembly.GetExecutingAssembly().Location);
+        System.Reflection.MethodInfo mi = asm.GetType(typeof(Runtime_68049_1).FullName).GetMethod(nameof(MainInner));
+        System.Type runtimeTy = asm.GetType(typeof(Runtime).FullName);
+        mi.Invoke(null, new object[]{System.Activator.CreateInstance(runtimeTy)});
+        return (int) s_result;


I would remove this test. It requires loading the assembly from file which means it has to be disabled in various configurations in issues.targets. Also I think the s_result is being set in the other instance of the type (the one loaded in the other ALC), not in this one, so it will always fail. Probably easier to just rely on the other test.

AndyAyersMS · 2022-04-15T21:42:01Z

Evidently we can't just copy the flags over:

Assert failure(PID 6704 [0x00001a30], Thread: 6592 [0x19c0]): Assertion failed '!"Extra flags on tree"' in 'ILGEN_0x1c02a9a5:Main():int' during 'Morph - Global' (IL size 58; hash 0xd149b15e; FullOpts)

Extra flags on tree [000122]: -----XG-------
[000122] n--XG+-N----                        *  IND       long
[000117] ------------                        --*  CNS_INT   byref  0

AndyAyersMS · 2022-04-15T22:33:48Z

The extra flag is the ~~GTF_IND_NONFAULTING~~ GTF_EXCEPT. We get here because we have

fgMorphArrayIndex (before remorph):
               [011944] ---XG-------              *  COMMA     long  
               [011936] ---X--------              +--*  BOUNDS_CHECK_Rng void  
               [004058] ---X-----U--              |  +--*  CAST_ovfl int <- ubyte <- uint
               [004057] ------------              |  |  \--*  CNS_INT   int    -1
               [011935] ---X--------              |  \--*  ARR_LENGTH int   
               [004056] ------------              |     \--*  LCL_VAR   ref    V11 loc3         
               [004059] n--XG-------              \--*  IND       long  
               [011943] ---X---N----                 \--*  ARR_ADDR  byref long[]
               [011942] ---X--------                    \--*  ADD       byref 
               [011932] ------------                       +--*  LCL_VAR   ref    V11 loc3         
               [011941] ---X--------                       \--*  ADD       long  
               [011939] ---X--------                          +--*  MUL       long  
               [011937] ---X-----U--                          |  +--*  CAST      long <- uint
               [011933] ---X-----U--                          |  |  \--*  CAST_ovfl int <- ubyte <- uint
               [011934] ------------                          |  |     \--*  CNS_INT   int    -1
               [011938] -------N----                          |  \--*  CNS_INT   long   8
               [011940] ------------                          \--*  CNS_INT   long   16

Folding binary operator with constant nodes into a comma throw:
               [004058] ---X-----U--              *  CAST_ovfl int <- ubyte <

and gtFoldExprConst basically turns this into

IND(COMMA(THROW(), 0))

so after the IND/COMMA interchange we end up with a non-faulting excepting indir off off zero which is also unreachable.

COMMA(THROW(), IND(0))

This eventually makes the flag checker uneasy.

This may be why the old code cleared GTF_IND_NONFAULTING but there's something unsavory about the whole business as most of the time we ought to be able to preserve that flag.

We perhaps should check for comma throw here and just nuke the indir entirely.

jakobbotsch · 2022-04-15T22:38:54Z

              [004059] n--XG-------              \--*  IND       long  
              [011943] ---X---N----                 \--*  ARR_ADDR  byref long[]
              [011942] ---X--------                    \--*  ADD       byref 
              [011932] ------------                       +--*  LCL_VAR   ref    V11 loc3         
              [011941] ---X--------                       \--*  ADD       long  
              [011939] ---X--------                          +--*  MUL       long  
              [011937] ---X-----U--                          |  +--*  CAST      long <- uint
              [011933] ---X-----U--                          |  |  \--*  CAST_ovfl int <- ubyte <- uint

It already looks strange to me here. How can an indirection have both GTF_IND_NONFAULTING and GTF_EXCEPT? Aren't those mutually exclusive?

AndyAyersMS · 2022-04-15T22:41:45Z

It already looks strange to me here. How can an indirection have both GTF_IND_NONFAULTING and GTF_EXCEPT? Aren't those mutually exclusive?

That's okay -- an array access is guaranteed to either throw an exception or give you and address you can safely dereference. Recall GTF_EXCEPT is a whole-tree property.

(What I wrote above may be a little off, I'm not yet sure how we get into the IND/COMMA interchange, since it looks like the IND is already under the COMMA. Need to do a bit more debugging).

Update: there's an intermediate tree state that isn't shown in the dumps. It would eappear there's some other part of morph that can transform COMMA(..., IND(x)) into IND(COMMA(..., x)) or something along those lines. Would be nice if there was a simpler repro.

               [009578] *--XG-------              *  IND       byte  
               [011574] ---XG+------              \--*  COMMA     byref 
               [011569] ---X-+------                 +--*  BOUNDS_CHECK_Rng void  
               [009575] -----+------                 |  +--*  CNS_INT   int    8
               [011568] ---X-+------                 |  \--*  ARR_LENGTH int   
               [009574] -----+------                 |     \--*  LCL_VAR   ref    V09 loc1         
               [011573] -----+-N----                 \--*  ARR_ADDR  byref byte[]
               [011572] -----+------                    \--*  ADD       byref 
               [011566] -----+------                       +--*  LCL_VAR   ref    V09 loc1         
               [011571] -----+------                       \--*  CNS_INT   long   24

jakobbotsch · 2022-04-15T22:46:20Z

That's okay -- an array access is guaranteed to either throw an exception or give you and address you can safely dereference. Recall GTF_EXCEPT is a whole-tree property.

Ah ok, but they are mutually exclusive if the child of the indirection does not have GTF_EXCEPT, which I suppose is what the checker complains about?

AndyAyersMS · 2022-04-15T22:51:50Z

Ah ok, but they are mutually exclusive if the child of the indirection does not have GTF_EXCEPT, which I suppose is what the checker complains about?

I need to debug through this part too as I don't quite see how it ends up complaining about what it complains about.

[Edit: you were right about this case, a non-faulting indir should not have GTF_EXCEPT set unless its address does].

AndyAyersMS · 2022-04-16T00:14:31Z

Debugged and the actual checker flag complaint is indeed about GTF_EXCEPT, and as you note it makes no sense to have this set on a GTF_IND_NONFAULTING indir if the base address doesn't also have it set.

dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Apr 15, 2022

ghost assigned AndyAyersMS Apr 15, 2022

SingleAccretion reviewed Apr 15, 2022

View reviewed changes

feedback

1bd4b24

SingleAccretion approved these changes Apr 15, 2022

View reviewed changes

jakobbotsch reviewed Apr 15, 2022

View reviewed changes

remove ALC-dependent test

9b87f0f

revise flag setting once again

dfb0840

jakobbotsch approved these changes Apr 16, 2022

View reviewed changes

AndyAyersMS merged commit 471a297 into dotnet:main 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: fix flags lossage in morph comma/ind interchange #68082

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

AndyAyersMS commented Apr 15, 2022

ghost commented Apr 15, 2022

AndyAyersMS commented Apr 15, 2022

SingleAccretion Apr 15, 2022

AndyAyersMS Apr 15, 2022

jakobbotsch Apr 15, 2022

AndyAyersMS Apr 15, 2022

AndyAyersMS commented Apr 15, 2022

AndyAyersMS commented Apr 15, 2022 •

edited

Loading

jakobbotsch commented Apr 15, 2022

AndyAyersMS commented Apr 15, 2022 •

edited

Loading

jakobbotsch commented Apr 15, 2022

AndyAyersMS commented Apr 15, 2022 •

edited

Loading

AndyAyersMS commented Apr 16, 2022

		op1->gtFlags \|= (addr->gtFlags & GTF_ALL_EFFECT);
		op1->gtFlags \|= treeFlags & (GTF_GLOB_REF \| GTF_IND_FLAGS);

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

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

Conversation

AndyAyersMS commented Apr 15, 2022

ghost commented Apr 15, 2022

AndyAyersMS commented Apr 15, 2022

SingleAccretion Apr 15, 2022

Choose a reason for hiding this comment

AndyAyersMS Apr 15, 2022

Choose a reason for hiding this comment

jakobbotsch Apr 15, 2022

Choose a reason for hiding this comment

AndyAyersMS Apr 15, 2022

Choose a reason for hiding this comment

AndyAyersMS commented Apr 15, 2022

AndyAyersMS commented Apr 15, 2022 • edited Loading

jakobbotsch commented Apr 15, 2022

AndyAyersMS commented Apr 15, 2022 • edited Loading

jakobbotsch commented Apr 15, 2022

AndyAyersMS commented Apr 15, 2022 • edited Loading

AndyAyersMS commented Apr 16, 2022

AndyAyersMS commented Apr 15, 2022 •

edited

Loading

AndyAyersMS commented Apr 15, 2022 •

edited

Loading

AndyAyersMS commented Apr 15, 2022 •

edited

Loading