Propagate assertions for more checked bounds by JosephTremoulet · Pull Request #11521 · dotnet/coreclr

JosephTremoulet · 2017-05-11T01:39:57Z

Generalize (and rename) the assertion/limit code for tracking array
lengths (which have until now been recognized by the VNFunc being
GT_ARR_LENGTH) to track all values that appear in the function as
length arguments to bounds chcks nodes. Add a hashtable to identify that
set of value numbers, m_checkedBoundVNs to the VNStore object, and
populate it during value-numbering. Wherever names were adjusted, the
generalization of "array length" is called "checked bound".

This allows the array bound check removal machinery to operate on span
bound checks as well.

JosephTremoulet · 2017-05-11T01:40:58Z

@briansull, @AndyAyersMS PTAL
/cc @dotnet/jit-contrib

This addresses #10050 with the exception that the value-number update that happens for compare nodes when one of their arguments is an array length that gets CSE'd does not apply in as many cases to spans, because we don't have the CSE candidate occurrence lists collected yet when we build the optCseCheckedBoundMap; that is left as a future possible improvement.

The Performance/CodeQuality/Span/Indexer test consists of several indexing patterns which we can use to compare the power of span bound check removal against array bound check removal. The effect of this change (on top of #11207) is:

Sub-Test	Effect of this change	Effect of this change plus #10453	Effect of this change plus fix for `optCseCheckedBoundMap` issue
Indexer1	bound check removed; array parity
Indexer2	no change (CSE doesn't update VN)	bound check removed; array parity	bound check removed; array parity
Indexer3	bound checked removed; array parity
Indexer4	bound checked removed; array parity
Indexer5	no change (afraid write could clobber span length)	bound checked removed; array parity
Indexer6	no change (afraid write could clobber span length)	bound checked removed; array parity
ReadOnlyIndexer1	bound check removed; array parity
ReadOnlyIndexer2	no change (CSE doesn't update VN)	bound check removed; array parity	bound check removed; array parity
WriteViaIndexer1	no change (afraid write could clobber span length)	bound checks on data[idx] removed, not on data[0]
WriteViaIndexer2	no change, but already have array parity
KnownSizeArray	no change, but already have array parity
KnownSizeCtor	no change, but already have array parity
KnownSizeCtor2	no change, but already have array parity
SameIndex1	no change, but already have array parity
SameIndex2	no change (afraid write could clobber span length)	bound checked removed; array parity
CoveredIndex1	no change, but already have array parity
CoveredIndex2	no change, but already have array parity
CoveredIndex3	no change, but already have array parity

No other diffs from this change (spans aren't used much in our test corpus)

briansull · 2017-05-11T01:46:06Z

+    // Cases where op1 holds the condition bound and op2 is 0.
+    // Loop condition like: "i < len == 0"
+    // Assertion: "i < len == false"
+    else if (vnStore->IsVNCompareCheckedBound(vn) &&


The comment block reads better like this:

// Cases where op1 holds the upper bound and op2 is 0. // Loop condition like: "i < bnd == 0" // Assertion: "i < bnd == false"

If you change this also change the previous comment block to match.

And a few more below...

AndyAyersMS

Looks good overall. Just some nits on comments and use of locals to cache things.

AndyAyersMS · 2017-05-11T01:52:16Z

        {
-            // Found an arrayLen feeding a compare that is a tracatable function of it;
-            // record this in the map so we can update the compare VN if the array length
+            // Found a checked bound feeding a compare that is a tracatable function of it;


Might as well fix this typo while you're at it: tracatable -> tractable

AndyAyersMS · 2017-05-11T01:55:14Z

-                        ValueNumStore::ArrLenArithBoundInfo info;
-                        ValueNum                            newCmpArgVN;
-                        if (vnStore->IsVNArrLenBound(oldCmpVN))
+                        ValueNumStore* vnStore  = m_pCompiler->vnStore;


Can you hoist the vnStore local defn up a few lines and use it in the new code you added above?

AndyAyersMS · 2017-05-11T01:59:54Z

    {
        // Get the array reference from the length.
-        arrRefVN = m_pCompiler->vnStore->GetArrForLenVn(uLimitVN);
+        ValueNum arrRefVN = m_pCompiler->vnStore->GetArrForLenVn(uLimitVN);


Maybe introduce a local for m_pCompiler->vnStore (up above somewhere suitable), it is used a dozen or more times below and would both make the code more readable and run a tiny bit faster.

AndyAyersMS · 2017-05-11T02:07:10Z

+    // Cases where op1 holds the condition bound and op2 is 0.
+    // Loop condition like: "i < len == 0"
+    // Assertion: "i < len == false"
+    else if (vnStore->IsVNCompareCheckedBound(vn) &&


If you change this also change the previous comment block to match.

AndyAyersMS · 2017-05-11T02:08:02Z

+    // Cases where op1 holds the condition bound and op2 is 0.
+    // Loop condition like: "i < len == 0"
+    // Assertion: "i < len == false"
+    else if (vnStore->IsVNCompareCheckedBound(vn) &&


And a few more below...

mikedn · 2017-05-11T04:58:14Z


                tree->gtVNPair = vnStore->VNPWithExc(vnStore->VNPForVoid(), excSet);
+
+                // Record non-constant value numbers that are used a the length argument to bounds checks, so that


are used as the length

- Add default key infos for int/unsigned - Call compGetMem through a forward-declared wrapper to avoid circular dependency problems if Compiler itself (or one of its dependencies) uses a SmallHashTable

Generalize (and rename) the assertion/limit code for tracking array lengths (which have until now been recognized by the VNFunc being GT_ARR_LENGTH) to track all values that appear in the function as length arguments to bounds chcks nodes. Add a hashtable to identify that set of value numbers, `m_checkedBoundVNs` to the VNStore object, and populate it during value-numbering. Wherever names were adjusted, the generalization of "array length" is called "checked bound". This allows the array bound check removal machinery to operate on span bound checks as well.

JosephTremoulet · 2017-05-12T01:14:29Z

Updated per feedback (and to fix build issue)

JosephTremoulet · 2017-05-15T20:26:28Z

Perf lab run came back, the indexer tests with diffs do indeed show improvement:

Test	Improvement
Indexer1	19%
Indexer3	18%
Indexer4	7%
ReadOnlyIndexer1	19%

JosephTremoulet · 2017-05-19T18:58:50Z

@dotnet-bot test this please

dnfclas added the cla-already-signed label May 11, 2017

JosephTremoulet requested review from AndyAyersMS and briansull May 11, 2017 01:40

briansull reviewed May 11, 2017

View reviewed changes

AndyAyersMS reviewed May 11, 2017

View reviewed changes

mikedn reviewed May 11, 2017

View reviewed changes

JosephTremoulet force-pushed the SpanAssert branch from b80e270 to 0a95122 Compare May 11, 2017 19:57

Make SmallHashTable more widely usable

cda51e4

- Add default key infos for int/unsigned - Call compGetMem through a forward-declared wrapper to avoid circular dependency problems if Compiler itself (or one of its dependencies) uses a SmallHashTable

JosephTremoulet force-pushed the SpanAssert branch from 0a95122 to da3e151 Compare May 11, 2017 20:41

JosephTremoulet force-pushed the SpanAssert branch from da3e151 to 72e126a Compare May 11, 2017 21:32

JosephTremoulet merged commit 4908b14 into dotnet:master May 20, 2017

JosephTremoulet deleted the SpanAssert branch May 20, 2017 02:26

karelz modified the milestone: 2.1.0 Aug 28, 2017

JosephTremoulet mentioned this pull request Jan 31, 2020

JIT optimization: value re-numbering in CSE doesn't help span loops as much as array loops dotnet/runtime#8177

Open


		tree->gtVNPair = vnStore->VNPWithExc(vnStore->VNPForVoid(), excSet);

		// Record non-constant value numbers that are used a the length argument to bounds checks, so that

Conversation

JosephTremoulet commented May 11, 2017

Uh oh!

JosephTremoulet commented May 11, 2017

Uh oh!

briansull May 11, 2017 • 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!

AndyAyersMS 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!

JosephTremoulet commented May 12, 2017

Uh oh!

JosephTremoulet commented May 15, 2017

Uh oh!

JosephTremoulet commented May 19, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

briansull May 11, 2017 •

edited

Loading