Enable checking of GTF_EXCEPT and GTF_ASG flags. by erozenfeld · Pull Request #13668 · dotnet/coreclr

erozenfeld · 2017-08-29T23:40:32Z

fgDebugCheckFlags is modified to check that GTF_EXCEPT and GTF_ASG are set precisely when needed.
It's also modified to handle several special operators correctly.

fgAddrCouldBeNull is updated to check for handles, implicit byref locals, and stack byrefs.

OperMayThrow is modified to handle several operators correctly.

GTF_IND_NONFAULTING is reused on operations for which OperIsIndir() is true and on GT_ARR_LENGTH.

Various places in morph are updated to set side effect flags correctly.

gtUpdateSideEffects is re-written so that it's precise for GTF_ASG and GTF_EXCEPT
and conservatively correct for the other side effects. It's now called from more places
to keep the flags up-to-date after transformations.

NoThrow in HelperCallProperties is updated and GTF_EXCEPT flag is set on helper calls according to
that property.

optRemoveRangeCheck is cleaned up and simplified.

fgDebugCheckFlags is modified to check that GTF_EXCEPT and GTF_ASG are set precisely when needed. It's also modified to handle several special operators correctly. fgAddrCouldBeNull is updated to check for handles, implicit byref locals, and stack byrefs. OperMayThrow is modified to handle several operators correctly. GTF_IND_NONFAULTING is reused on operations for which OperIsIndir() is true and on GT_ARR_LENGTH. Various places in morph are updated to set side effect flags correctly. gtUpdateSideEffects is re-written so that it's precise for GTF_ASG and GTF_EXCEPT and conservatively correct for the other side effects. It's now called from more places to keep the flags up-to-date after transformations. NoThrow in HelperCallProperties is updated and GTF_EXCEPT flag is set on helper calls according to that property. optRemoveRangeCheck is cleaned up and simplified.

erozenfeld · 2017-08-29T23:41:03Z

SPC codesize numbers:

Summary:
(Note: Lower is better)
Total bytes of diff: -360 (-0.01 % of base)
    diff is an improvement.
Total byte diff includes 0 bytes from reconciling methods
        Base had    0 unique methods,        0 unique bytes
        Diff had    0 unique methods,        0 unique bytes
Top file improvements by size (bytes):
        -360 : System.Private.CoreLib.dasm (-0.01 % of base)
1 total files with size differences (1 improved, 0 regressed), 0 unchanged.
Top method regessions by size (bytes):
          64 : System.Private.CoreLib.dasm - CustomAttributeBuilder:InitCustomAttributeBuilder(ref,ref,ref,ref,ref,ref):this
          35 : System.Private.CoreLib.dasm - ILGenerator:Emit(struct,ref):this (8 methods)
          21 : System.Private.CoreLib.dasm - DynamicILGenerator:Emit(struct,ref):this (6 methods)
          19 : System.Private.CoreLib.dasm - TypeLoadException:SetMessageField():this
          18 : System.Private.CoreLib.dasm - TypeAnalysis:.ctor(ref,ref,ref):this
Top method improvements by size (bytes):
        -133 : System.Private.CoreLib.dasm - GenericEqualityComparer`1:LastIndexOf(ref,struct,int,int):int:this (19 methods)
        -133 : System.Private.CoreLib.dasm - GenericEqualityComparer`1:IndexOf(ref,struct,int,int):int:this (19 methods)
         -78 : System.Private.CoreLib.dasm - GenericArraySortHelper`1:DownHeap(ref,int,int,int) (18 methods)
         -72 : System.Private.CoreLib.dasm - ObjectEqualityComparer`1:LastIndexOf(ref,struct,int,int):int:this (11 methods)
         -72 : System.Private.CoreLib.dasm - ObjectEqualityComparer`1:IndexOf(ref,struct,int,int):int:this (11 methods)
59 total methods with size differences (28 improved, 31 regressed), 16218 unchanged.

erozenfeld · 2017-08-29T23:44:16Z

Benchmarks codesize numbers:

Total bytes of diff: 10 (0.01 % of base)
    diff is a regression.
Total byte diff includes 0 bytes from reconciling methods
        Base had    0 unique methods,        0 unique bytes
        Diff had    0 unique methods,        0 unique bytes
Top file regressions by size (bytes):
          20 : BenchmarksGame\fasta\fasta\fasta.dasm (0.62 % of base)
          16 : BenchmarksGame\fastaredux\fastaredux\fastaredux.dasm (0.50 % of base)
Top file improvements by size (bytes):
         -20 : BenchmarksGame\k-nucleotide\k-nucleotide\k-nucleotide.dasm (-0.42 % of base)
          -3 : BenchI\NDhrystone\NDhrystone\NDhrystone.dasm (-0.14 % of base)
          -3 : BenchmarksGame\spectralnorm\spectralnorm\spectralnorm.dasm (-0.14 % of base)
5 total files with size differences (3 improved, 2 regressed), 51 unchanged.
Top method regessions by size (bytes):
          27 : BenchmarksGame\fasta\fasta\fasta.dasm - Fasta:Bench()
          16 : BenchmarksGame\fastaredux\fastaredux\fastaredux.dasm - FastaRedux:Bench()
           2 : BenchmarksGame\k-nucleotide\k-nucleotide\k-nucleotide.dasm - knucleotide:Main(ref):int
Top method improvements by size (bytes):
         -22 : BenchmarksGame\k-nucleotide\k-nucleotide\k-nucleotide.dasm - knucleotide:CountFrequency(ref,int):ref
          -4 : BenchmarksGame\fasta\fasta\fasta.dasm - Fasta:MakeRepeatFasta(ref,ref,ref,int,ref)
          -3 : BenchI\NDhrystone\NDhrystone\NDhrystone.dasm - NDhrystone:Proc0()
          -3 : BenchmarksGame\fasta\fasta\fasta.dasm - Fasta:MakeRandomFasta(ref,ref,ref,int,ref)
          -3 : BenchmarksGame\spectralnorm\spectralnorm\spectralnorm.dasm - SpectralNorm:Approximate(int):double:this
8 total methods with size differences (5 improved, 3 regressed), 419 unchanged.

erozenfeld · 2017-08-29T23:47:33Z

Frameworks codesize numbers:

Summary:
(Note: Lower is better)
Total bytes of diff: 1199 (0.01 % of base)
    diff is a regression.
Total byte diff includes 0 bytes from reconciling methods
        Base had    0 unique methods,        0 unique bytes
        Diff had    0 unique methods,        0 unique bytes
Top file regressions by size (bytes):
         532 : Microsoft.CodeAnalysis.dasm (0.07 % of base)
         528 : System.Linq.Parallel.dasm (0.09 % of base)
         230 : Microsoft.CodeAnalysis.VisualBasic.dasm (0.01 % of base)
         228 : Microsoft.CodeAnalysis.CSharp.dasm (0.01 % of base)
          91 : System.Collections.Immutable.dasm (0.06 % of base)
Top file improvements by size (bytes):
        -360 : System.Private.CoreLib.dasm (-0.01 % of base)
         -30 : System.Runtime.Extensions.dasm (-0.02 % of base)
         -29 : System.Linq.Expressions.dasm (-0.01 % of base)
         -20 : System.Net.Security.dasm (-0.01 % of base)
         -17 : System.ComponentModel.Annotations.dasm (-0.05 % of base)
27 total files with size differences (13 improved, 14 regressed), 52 unchanged.
Top method regessions by size (bytes):
         520 : System.Linq.Parallel.dasm - QueryOpeningEnumerator`1:OpenQuery():this (26 methods)
          64 : System.Private.CoreLib.dasm - CustomAttributeBuilder:InitCustomAttributeBuilder(ref,ref,ref,ref,ref,ref):this
          58 : Microsoft.CodeAnalysis.dasm - ReferenceIndexerBase:Visit(ref):this (17 methods)
          50 : System.Collections.Immutable.dasm - Enumerator:.ctor(ref):this (5 methods)
          49 : Microsoft.CodeAnalysis.VisualBasic.dasm - Binder:AdjustReceiverTypeOrValue(ref,ref,bool,bool,ref,byref,byref):ref:this
Top method improvements by size (bytes):
        -133 : System.Private.CoreLib.dasm - GenericEqualityComparer`1:LastIndexOf(ref,struct,int,int):int:this (19 methods)
        -133 : System.Private.CoreLib.dasm - GenericEqualityComparer`1:IndexOf(ref,struct,int,int):int:this (19 methods)
         -78 : System.Private.CoreLib.dasm - GenericArraySortHelper`1:DownHeap(ref,int,int,int) (18 methods)
         -75 : Microsoft.CodeAnalysis.VisualBasic.dasm - SourceModuleSymbol:BindImports(struct):ref:this
         -72 : System.Private.CoreLib.dasm - ObjectEqualityComparer`1:LastIndexOf(ref,struct,int,int):int:this (11 methods)
278 total methods with size differences (94 improved, 184 regressed), 66022 unchanged.

erozenfeld · 2017-08-29T23:48:30Z

I measured instructions retired TP impact when crossgen-ing SPC:
minopts: 0.09% regression
fullopts: 0.13% regression

erozenfeld · 2017-08-29T23:49:12Z

@pgavlin @JosephTremoulet @briansull @CarolEidt @dotnet/jit-contrib PTAL

erozenfeld · 2017-08-29T23:52:50Z

The three main sources of regressions:

There are places where we now set GTF_EXCEPT flag where previously we didn't. In particular, dereferencing struct fields requires this flag unless we can prove the byref is non-null.
Simplified call args can affect arg sorting.
Removing GTF_ASG may affect setting of GTF_REVERSE_OP on an ancestor assignment node.

erozenfeld · 2017-08-30T00:21:31Z

@dotnet-bot test Ubuntu16.04 arm Cross Debug Build

CarolEidt

Looks good overall. I have a few suggestions.

CarolEidt · 2017-08-30T00:21:30Z


-inline GenTreeCall* Compiler::gtNewHelperCallNode(unsigned helper, var_types type, unsigned flags, GenTreeArgList* args)
+//------------------------------------------------------------------------------
+// gtNewHelperCallNode : Helper to create a call helper node.


Suggestion: I would simply say "Create a node that calls a helper method.". The repetition of "helper" is somewhat confusing, and although a number of these gtNew methods are described as helpers, I'm not sure I'd really call them that.

CarolEidt · 2017-08-30T00:25:48Z

+    unsigned mask = GTF_COMMON_MASK;
+    if (this->OperIsIndirOrArrLength() && OperIsIndir(oper))
+    {
+        mask |= GTF_IND_NONFAULTING;


Suggestion: at first glance this is confusing because it almost looks like you're unconditionally setting this flag (though if one reads a bit further it's clear). It wouldn't hurt to add a comment such as "If this is an indirection, we want to inherit the value of the GTF_IND_NONFAULTING flag in addition to anything set in GTF_COMMON_MASK or something to that effect.

CarolEidt · 2017-08-30T00:27:56Z

@@ -1529,8 +1601,13 @@ inline void GenTree::ChangeOper(genTreeOps oper, ValueNumberUpdate vnUpdate)

 inline void GenTree::ChangeOperUnchecked(genTreeOps oper)


Would it make sense for ChangeOper to call this, so that the logic (such as it is) doesn't have to be duplicated?

ChangeOper calls SetOper while ChangeOperUnchecked calls SetOperRaw so ChangeOper can's call ChangeOperUnchecked.

CarolEidt · 2017-08-30T00:31:24Z

    {
        VarSetOps::OldStyleClearD(this, optCopyPropKillSet);

+        bool updateFlags = false;


This seems to be unused

CarolEidt · 2017-08-30T00:36:57Z


-    return gtNewHelperCallNode(helper, type, callFlags, argList);
+    GenTreeCall* result = gtNewHelperCallNode(helper, type, argList);
+    result->gtFlags |= callFlags;


I believe that this, and the setting of callFlags above are now redundant, as this is done in gtNewHelperCallNode(), right?

This line is not redundant since in this method callFlags can also have GTF_CALL_HOISTABLE. The setting of GTF_EXCEPT above is redundant and I will remove it.

CarolEidt · 2017-08-30T00:40:27Z

+//    addr     -  Address to check
 //
+// Return Value:
+//    True is address could be null; false otherwise


Perhaps should be "True if address ..."?

CarolEidt · 2017-08-30T01:13:23Z

+// gtUpdateSideEffects: Update the side effects for statement tree nodes.
+//
+// Arguments:
+//    stmt            - The statement to update side effects on


You need a description of the tree argument. Also, since tree is unused in the then clause below, and stmt is unused in the else clause, would it make sense to have two different versions of this method, especially since their behavior is quite different - in one case an entire statement is updated, and in another case, only the direct children of tree, and its ancestors.

CarolEidt · 2017-08-30T01:17:22Z

-            // any struct arguments.
-            // i.e. assert(((call->gtFlags & GTF_EXCEPT) != 0) || ((args->Current()->gtFlags & GTF_EXCEPT) == 0)
-            flagsSummary |= (args->Current()->gtFlags & GTF_EXCEPT);
-


Glad to see this go!

CarolEidt · 2017-08-30T01:21:21Z

+                else
+                {
+                    src->gtFlags |= GTF_IND_NONFAULTING;
+                }


Not a big deal, but the above pattern appears a lot - it might be nice to extract it into a method.

mikedn · 2017-08-30T05:11:11Z


    GenTreePtr gtNewIndexRef(var_types typ, GenTreePtr arrayOp, GenTreePtr indexOp);

+    GenTreeArrLen* gtNewArrLen(var_types typ, GenTreePtr arrayOp, int lenOffset);


Can we stick to using GenTree* in new code?

mikedn · 2017-08-30T05:15:02Z

+
+inline GenTreeCall* Compiler::gtNewHelperCallNode(unsigned helper, var_types type, GenTreeArgList* args)
 {
+    unsigned     flags  = this->s_helperCallProperties.NoThrow((CorInfoHelpFunc)helper) ? 0 : GTF_EXCEPT;


What's this-> for? Looks strange considering that you're accessing a static member.

mikedn · 2017-08-30T05:16:53Z

+//    curSsaName  -  The map from lclNum to its recently live definitions as a stack
+//
+// Return Value:
+//    true if copy propagation was performed for the tree; false otherwise.


It looks like the function doesn't return anything.

mikedn · 2017-08-30T05:18:54Z

        }
 #endif
-        break;
+        return;


This change looks odd, considering that the function ends right after the loop and there's even a useless return after the loop.

mikedn · 2017-08-30T05:25:37Z


+        case GT_CALL:
+
+            GenTreeCall* call;


This variable seems unnecessary, it's used only once.

mikedn · 2017-08-30T05:27:23Z

+        }
+    }
+
+    genTreeOps oper = tree->OperGet();


Unused variable?

mikedn · 2017-08-30T05:31:10Z


+    if (tree->OperMayThrow(this))
+    {
+        tree->gtFlags |= GTF_EXCEPT;


Maybe we should add some flags setters to GenTree, this stuff keeps repeating.

tree->SetFlag(GTF_EXCEPT, tree->OperMayThrow(this)) for example.

mikedn · 2017-08-30T05:33:32Z

    if (argx->gtOper == GT_OBJ)
    {
        argx->gtFlags &= ~(GTF_ALL_EFFECT) | (argx->AsBlk()->Addr()->gtFlags & GTF_ALL_EFFECT);
+        if (argx->OperMayThrow(this))


I'm not usually a fan of ?: but sometimes it seems useful: argx->gtFlags |= argx->OperMayThrow(this) ? GTF_EXCEPT : GTF_IND_NONFAULTING;

mikedn · 2017-08-30T05:40:32Z

-                dest->gtFlags |= (GTF_EXCEPT | GTF_GLOB_REF | GTF_IND_TGTANYWHERE);
-                tree->gtFlags |= (GTF_EXCEPT | GTF_GLOB_REF | GTF_IND_TGTANYWHERE);
+                dest->gtFlags |= (GTF_GLOB_REF | GTF_IND_TGTANYWHERE);
+                tree->gtFlags |= (GTF_GLOB_REF | GTF_IND_TGTANYWHERE);


Is it correct to set GTF_IND_TGTANYWHERE on tree? Seems like it is a GT_ASG, not a GT_IND.

It's pre-existing but I don't think it's correct. I'll try to remove it.

mikedn · 2017-08-30T05:48:34Z

+            case CORINFO_HELP_MON_ENTER:
+            case CORINFO_HELP_MON_EXIT:
+            case CORINFO_HELP_MON_ENTER_STATIC:
+            case CORINFO_HELP_MON_EXIT_STATIC:


Are the monitor helpers really "no throw"? Seems like "exit" could throw SynchronizationLockException.

briansull · 2017-08-30T18:26:40Z

+//    New GT_IND node
+
+inline GenTreePtr Compiler::gtNewIndir(var_types typ, GenTreePtr addr)
+{


GenTreePtr => GenTree*

erozenfeld · 2017-08-30T19:20:03Z

@CarolEidt @mikedn @briansull Thank you for the review. I pushed a commit that addresses your comments. PTAL.

erozenfeld · 2017-08-31T00:13:53Z

@dotnet-bot test Windows_NT x64 Release Priority 1 Build and Test
@dotnet-bot test Windows_NT arm Cross Checked Build and Test

erozenfeld · 2017-08-31T06:25:47Z

@dotnet-bot test Windows_NT jitstress1
@dotnet-bot test Windows_NT jitstress2_jitstressregs8

erozenfeld · 2017-08-31T06:27:04Z

@dotnet-bot test Windows_NT arm Cross Checked Build and Test

JosephTremoulet

Thanks for cleaning this up! Looks good w/ a few comments.

JosephTremoulet · 2017-08-31T15:58:43Z

    assert(!OperIsConst(oper)); // use ChangeOperLeaf() instead

+    unsigned mask = GTF_COMMON_MASK;
+    if (this->OperIsIndirOrArrLength() && OperIsIndir(oper))


Why is this assymetric? I presume we never ChangeOper from vanilla indir to array length, but if we did, wouldn't we want to propagate the flag?

JosephTremoulet · 2017-08-31T15:59:14Z

 inline void GenTree::ChangeOperUnchecked(genTreeOps oper)
 {
+    unsigned mask = GTF_COMMON_MASK;
+    if (this->OperIsIndirOrArrLength() && OperIsIndir(oper))


Same as above

JosephTremoulet · 2017-08-31T16:13:19Z

                                               //             contribution to its argument).
 #define GTF_IND_VOLATILE            0x40000000 // GT_IND   -- the load or store must use volatile sematics (this is a nop on X86)
-#define GTF_IND_NONFAULTING         0x20000000 // GT_IND   -- An indir that cannot fault.
+#define GTF_IND_NONFAULTING         0x20000000 // Operations for which OperIsIndirOrArrLength() is true  -- An indir that cannot fault.


I'd recommend defining GTF_ARRLEN_NONFAULTING where the other GTF_ARRLEN flags are defined and adding a static_assert_no_msg that the two flags agree -- that'll make it easier not to mistakenly try to re-use that bit if somebody goes to add another flag for array length nodes in the future. We currently do the same for GTF_FLD_VOLATILE and GTF_CLS_VAR_VOLATILE (and the static assertion is over by their use in morph that requires them to be the same).

erozenfeld · 2017-08-31T18:39:37Z

@JosephTremoulet Good points, thanks. I pushed a commit that addresses your comments.

JosephTremoulet · 2017-08-31T18:47:21Z


 #define GTF_ARR_BOUND_INBND         0x80000000 // GT_ARR_BOUNDS_CHECK -- have proved this check is always in-bounds

+#define GTF_ARRLEN_NONFAULTING      0x20000000 // GT_ARR_LENGTH  -- An array length operation that cannot fault. Same as GT_IND_NONFAULTING.


Mind moving this down a line to be below GTF_ARRLEN_ARR_IDX? The flags in this file are typically listed in descending numerical order within each group.

Perfect, thanks.

pgavlin

This generally looks good, just a couple questions.

It does look like this primarily targets the frontend: do you plan on adding side-effect checks to LIR::Range::CheckLIR() as a follow-up change?

pgavlin · 2017-08-31T19:04:37Z

                break;

+            case GT_MEMORYBARRIER:
+                chkFlags |= GTF_GLOB_REF | GTF_ASG;


I'm a bit surprised that GT_MEMORYBARRIER does not imply GT_ORDER_SIDEEFF...

I'm surprised by that too but the meaning of GTF_ORDER_SIDEEFF and GTF_GLOB_REF is not very well defined at the moment so I deferred the work to fix and check them and just kept the existing behavior.

GTF_ORDER_SIDEEFF is well defined: it is a full compiler barrier. I suppose that when you say that its meaning is not well defined you're referring to the fact that we don't always check it?

I don't think it's currently always used that way. In any case, checking GTF_ORDER_SIDEFF is a future work item and I'll investigate why it's not set on GT_MEMORYBARRIER at that time.

pgavlin · 2017-08-31T19:06:38Z

+
+            CorInfoHelpFunc helper;
+            helper = comp->eeGetHelperNum(this->AsCall()->gtCallMethHnd);
+            if ((helper == CORINFO_HELP_UNDEF) || !comp->s_helperCallProperties.NoThrow(helper))


Why not just

return (helper == CORINFO_HELP_UNDEF) || !comp->s_helperCallProperties.NoThrow(helper);

?

No reason. I pushed a commit with an update.

erozenfeld · 2017-08-31T19:30:17Z

I'll include adding side-effect checks to LIR::Range::CheckLIR() to my list of follow-up changes.

pgavlin · 2017-08-31T19:48:22Z

I'll include adding side-effect checks to LIR::Range::CheckLIR() to my list of follow-up changes.

Cool. There is a fundamental difference between side-effects in HIR and LIR in that the flags on a node do not need to be a summary of that node's side effects along with those of its operands: instead, the flags for a node should reflect only that node's side effects.

erozenfeld · 2017-08-31T23:04:03Z

GC.API.NoGCRegion.NoGC.NoGC failed in Ubuntu x64 Checked Build and Test . I see this failure in other PRs, e.g., #13728, #13576. @Maoni0 Is this a known issue?

erozenfeld · 2017-08-31T23:21:23Z

Rerunning the job. Link to the failure: https://ci.dot.net/job/dotnet_coreclr/job/master/job/checked_ubuntu_tst_prtest/9021/

erozenfeld · 2017-08-31T23:21:39Z

@dotnet-bot test Ubuntu x64 Checked Build and Test

Maoni0 · 2017-08-31T23:40:48Z

no, I was not aware this test was failing. Is this run with other tests? 'cause it's not supposed to. I am not sure if some test config has changed... @swgillespie do you know anything about this?

swgillespie · 2017-08-31T23:57:53Z

I was not aware either - I'll take a look. Get Outlook for iOS<https://aka.ms/o0ukef>

…

________________________________ From: Maoni Stephens <notifications@github.com> Sent: Thursday, August 31, 2017 4:40:59 PM To: dotnet/coreclr Cc: Sean Gillespie; Mention Subject: Re: [dotnet/coreclr] Enable checking of GTF_EXCEPT and GTF_ASG flags. (#13668) no, I was not aware this test was failing. Is this run with other tests? 'cause it's not supposed to. I am not sure if some test config has changed... @swgillespie<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fswgillespie&data=02%7C01%7Csegilles%40microsoft.com%7Ca2bf6f6c2a584d54403308d4f0c9bca9%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636398196628670598&sdata=qyXaKop0Aq%2FEJWCtIGqJV04Otf5apWAVleK9DGzOgJc%3D&reserved=0> do you know anything about this? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fdotnet%2Fcoreclr%2Fpull%2F13668%23issuecomment-326448672&data=02%7C01%7Csegilles%40microsoft.com%7Ca2bf6f6c2a584d54403308d4f0c9bca9%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636398196628670598&sdata=lE9zlxCG2XCLK7nYNqAV5CCVfQLMI5l1flzzrxNBXgQ%3D&reserved=0>, or mute the thread<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAByQKCt-Nf6r3Dv7vX3ZPonTUhS0Od7aks5sd0SKgaJpZM4PGr6W&data=02%7C01%7Csegilles%40microsoft.com%7Ca2bf6f6c2a584d54403308d4f0c9bca9%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636398196628670598&sdata=i0YmGa6FTEztcN%2FG%2FgjOBG9KkSX6Fc0W3mAXcd%2BkFlg%3D&reserved=0>.

erozenfeld · 2017-09-01T00:07:40Z

@dotnet-bot test Ubuntu x64 Checked Build and Test

erozenfeld · 2017-09-01T02:49:55Z

@dotnet-bot test Ubuntu x64 Checked Build and Test

jkotas · 2017-09-01T03:11:32Z

@maonis @swgillespie I have opened #13736 on the GC.API.NoGCRegion.NoGC.NoGC test failure

swgillespie · 2017-09-01T03:22:48Z

@jkotas thanks!

dnfclas added the cla-already-signed label Aug 29, 2017

CarolEidt reviewed Aug 30, 2017

View reviewed changes

mikedn reviewed Aug 30, 2017

View reviewed changes

Comment thread src/jit/gentree.cpp Outdated

}

}

genTreeOps oper = tree->OperGet();

Copy link
Copy Markdown

mikedn Aug 30, 2017

Choose a reason for hiding this comment

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

Unused variable?

mikedn reviewed Aug 30, 2017

View reviewed changes

briansull reviewed Aug 30, 2017

View reviewed changes

Address code review feedback.

f38b45b

CarolEidt approved these changes Aug 30, 2017

View reviewed changes

JosephTremoulet approved these changes Aug 31, 2017

View reviewed changes

Additional code review feedback.

f85d335

JosephTremoulet reviewed Aug 31, 2017

View reviewed changes

Move GTF_ARRLEN_NONFAULTING down.

7121a43

pgavlin approved these changes Aug 31, 2017

View reviewed changes

More review feedback.

b4eda94

erozenfeld merged commit 8a33888 into dotnet:master Sep 1, 2017

erozenfeld deleted the SideEffectFlags branch September 1, 2017 04:30

This was referenced Jan 31, 2020

RyuJIT: Improve checking of GTF_CALL flag dotnet/runtime#10246

Closed

RyuJIT: Improve checking of GTF_GLOB_REF flags dotnet/runtime#10247

Open

RyuJIT: Improve checking of GTF_ORDER_SIDEEFF flags dotnet/runtime#10248

Open

		@@ -1529,8 +1601,13 @@ inline void GenTree::ChangeOper(genTreeOps oper, ValueNumberUpdate vnUpdate)

		inline void GenTree::ChangeOperUnchecked(genTreeOps oper)


		GenTreePtr gtNewIndexRef(var_types typ, GenTreePtr arrayOp, GenTreePtr indexOp);

		GenTreeArrLen* gtNewArrLen(var_types typ, GenTreePtr arrayOp, int lenOffset);


		#define GTF_ARR_BOUND_INBND 0x80000000 // GT_ARR_BOUNDS_CHECK -- have proved this check is always in-bounds

		#define GTF_ARRLEN_NONFAULTING 0x20000000 // GT_ARR_LENGTH -- An array length operation that cannot fault. Same as GT_IND_NONFAULTING.

Conversation

erozenfeld commented Aug 29, 2017

Uh oh!

erozenfeld commented Aug 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erozenfeld commented Aug 29, 2017

Uh oh!

erozenfeld commented Aug 29, 2017

Uh oh!

erozenfeld commented Aug 29, 2017

Uh oh!

erozenfeld commented Aug 29, 2017

Uh oh!

erozenfeld commented Aug 29, 2017

Uh oh!

erozenfeld commented Aug 30, 2017

Uh oh!

CarolEidt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erozenfeld commented Aug 30, 2017

Uh oh!

erozenfeld commented Aug 31, 2017

Uh oh!

erozenfeld commented Aug 31, 2017

Uh oh!

erozenfeld commented Aug 31, 2017

Uh oh!

JosephTremoulet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erozenfeld commented Aug 29, 2017 •

edited

Loading

erozenfeld Aug 31, 2017 •

edited

Loading