[WIP] Support for tracking Exception Sets with ValueNumbers #19284

briansull · 2018-08-03T23:38:56Z

Added support to create and track Exception sets with ValueNumbers
Some cleanup in ValueNumbering as well.
The Asm Diffs produced are fairly minimal with this set of changes.
Fixes #8648

mikedn · 2018-08-06T19:08:06Z

src/jit/valuenum.cpp

                if (IsIntZero(v1))
                {
-                    *pExcSet = VNExcSetSingleton(VNForFunc(TYP_REF, VNF_DivideByZeroExc));
+                    *pExcSet = VNExcSetSingleton(VNForFunc(TYP_REF, VNF_DivideByZeroExc, VNForVoid()));


Is VNForVoid correct here? The description of VNF_DivideByZeroExc is the divisor value so it seems that here it should the VN of constant 0. Same comment for the next 2 changes bellow.

mikedn · 2018-08-06T19:50:31Z

src/jit/assertionprop.cpp

+    // ValueNumber indicates that this node evaluates to a constant
+
+    // But we could have an integer division by zero
+    if (tree->OperMayThrow(this))


Hmm, why is this needed?

Because the Norm value computed for integer division by zero is the constant zero.
It is problematic is serveral places where we think we have a constant but instead we have a divide by 0.
I will look into changing how we ValueNumber a division by zero,.

mikedn · 2018-08-06T19:52:47Z

src/jit/assertionprop.cpp

-    ValueNum vn = op1->gtVNPair.GetConservative();
+    ValueNum vn      = vnStore->VNNormVal(op1->gtVNPair.GetConservative());
+    ValueNum relopVN = vnStore->VNNormVal(relop->gtVNPair.GetConservative());
+    ValueNum op2VN   = vnStore->VNNormVal(relop->gtVNPair.GetConservative());


relop should be op2

Yes, Thanks

mikedn · 2018-08-06T19:53:10Z

src/jit/assertionprop.cpp

    GenTree* op2 = relop->gtGetOp2();

-    ValueNum vn = op1->gtVNPair.GetConservative();
+    ValueNum vn      = vnStore->VNNormVal(op1->gtVNPair.GetConservative());


Might be nice to rename to op1VN

mikedn · 2018-08-06T19:57:40Z

src/jit/assertionprop.cpp

+    // We typically want to use the Normal ValueNumber when checking for constants
+    // except for the nodes that might generate an exception like GT_DIV, GT_MOD, etc...
+    //
+    if (!tree->OperMayThrow(this))


So I guess this is needed because if we have a VN exception set we don't really know if it comes from the node itself or from its children?

Yes that is why I added this,
And Integer Divide is an oper than can Throw

mikedn · 2018-08-06T20:02:40Z

src/jit/assertionprop.cpp


    // If the assertion involves "op2" and it is a constant, then check if "op1" also has a constant value.
-    if (vnStore->IsVNConstant(op2->gtVNPair.GetConservative()))
+    ValueNum vnCns = vnStore->VNNormVal(op2->gtVNPair.GetConservative());


There are so many of these VNNormVal that perhaps we should add a shortcut to Compiler - ValueNum Compiler::vnConservativeNormVal(GenTree*);

Yes, I will be adding heper functions:

vnConservativeNormVal
vnLiberalNormVal

mikedn · 2018-08-06T20:07:35Z

src/jit/optcse.cpp

+    //
+    ValueNum vnLibNorm = vnStore->VNNormVal(vnLib);
+
+    // We assign either vnLib or vnLinNorm as the hash key


vnLinNorm -> vnLibNorm

mikedn · 2018-08-06T20:08:58Z

src/jit/optcse.cpp

+        // and the parent comma as the CSE use (but with a different exc set)
+        // This would prevent us from making any CSE with the comma
+        //
+        assert(vnLibNorm == vnStore->VNNormVal(vnOp2Lib)); // (We don't need to assert this)


The comment after the assert seems strange

mikedn · 2018-08-06T20:21:11Z

src/jit/optcse.cpp

+// Afterwards the set of nodes in the 'sideEffectList' are preserved and
+// all other nodes are removed and have their ref counts decremented
+//
+#ifdef DEBUG


It does not seem useful to introduce this variable for a single use. And the ifdef messes up the comment above.

Run clang format

Added methods VNConservativeNormVal() and VNLiberalNormVal() Renamed the local vn to op1VN in optCreateJTrueBoundsAssertion Other changes: For optCSE we now use set Union and Intersection and track the full set of exceptions promised by all defs using defExcSetPromise Implemented in ValueNum.cpp: new method VNExcSetIntersection to compute the intersection of two exception sets new method VNNegOneForType used by the new GT_MOD folding checks. updated VNExcSetUnion to add checks for ascending order VNCheckAscending for checking that setys are in ascending order Changed the value numbering for GT_DIV and GT_MOD, no longer constant fold them when they are throwing an exception. Deprecated STR_VN, it is replace with FMT_VN

Added overflowChecking arg for GetVNFuncForOper This method now supports new VNF's for ADD,SUB and MUL with overflow checking Renamed EvalOpIntegral to EvalOpSpecialized has it uses template speclization Moved the template method method to preceed the generic instantiation of T Cleaned up the implementation of EvalOp<T> it now only handles three op GT_ADD, GT_SUB and GT_MUL. We use EvalOPSpecialized to handle GT_DIV and GT_MOD RelOP are handled by EvalComparison and we now have support for unsigned compares as well as unordered flowting point compares. Implemented the floating of compares with NaN's Operations that throw exceptions are not folded to return 0 with an exception set. We now annotated div/Mod with both exceptions VNF_DivideByZeroExc and VNF_ArithmeticExc added supportfow new VNF's: CastOvf, ADD_UN,ADD_OVF and ADD_UN_OVF (also the SUB and MUL versions)

…the INT64 instantiation Fixed an issue with mismatch types when expanding a GT_MUL by 15

BruceForstall · 2018-09-05T21:04:04Z

@briansull Is this still WIP? Is it ready to be reviewed, or are you still working on it? (btw, it has merge conflicts now)

briansull · 2018-09-05T21:31:05Z

I decided to stage these changes by checking this stuff using an series of smaller change sets.
Currently I have this part our for review: #19720

briansull · 2018-10-10T18:06:48Z

Checked in with #20129

mikedn mentioned this pull request Aug 5, 2018

Add more VB operator tests dotnet/corefx#31320

Merged

mikedn reviewed Aug 6, 2018

View reviewed changes

briansull force-pushed the vn-except-support branch 2 times, most recently from 7058d54 to ac99312 Compare August 17, 2018 19:49

briansull added 5 commits August 17, 2018 20:09

Current work on Value Number with exception sets

fba89f7

Run clang format

Removed pExcSet from EvalOp and EvalOpSpecialized

b82c5b5

Added EvalOpInt since C++ automatic argument coercion makes it go to …

71acf22

…the INT64 instantiation Fixed an issue with mismatch types when expanding a GT_MUL by 15

briansull force-pushed the vn-except-support branch from 5579bfe to 71acf22 Compare August 18, 2018 03:10

briansull added 2 commits August 17, 2018 20:18

fix for 32-bit builds

0cae19e

Fix for 32-bit crossgen assert

b052110

briansull closed this Oct 10, 2018

[WIP] Support for tracking Exception Sets with ValueNumbers #19284

[WIP] Support for tracking Exception Sets with ValueNumbers #19284

Uh oh!

Conversation

briansull commented Aug 3, 2018

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!

briansull Aug 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

BruceForstall commented Sep 5, 2018

Uh oh!

briansull commented Sep 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

briansull commented Oct 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

briansull Aug 14, 2018 •

edited

Loading

briansull commented Sep 5, 2018 •

edited

Loading