Maintain sorted preds part 1 by jashook · Pull Request #13322 · dotnet/coreclr

jashook · 2017-08-10T23:07:05Z

This adds asserts to enforce sorted pred lists and changes fgReplacePred
to correctly maintain sorted order when replacing a pred in the list.

Note there are other areas that will break the sorted order of the Pred list. Therefore, this change should not yet be merged. However, it is worth starting a review for what it already fixes.

ptal @BruceForstall @CarolEidt
/cc @RussKeldorph

This adds asserts to enforce sorted pred lists and changes fgReplacePred to correctly maintain sorted order when replacing a pred in the list.

jashook · 2017-08-11T21:37:10Z

No longer a WIP. Open question on whether to merge until we can assert preds are sorted or not.

CarolEidt

It would be useful to see some throughput numbers. Does this method show up at all? Might it be worthwhile to first handle the simple case where the oldPred and newPred have the same sort order?

CarolEidt · 2017-08-11T21:58:04Z

+
+        for (flowList* pred = block->bbPreds; pred != nullptr; pred = pred->flNext)
+        {
+            // TODO: Assert here when preds are actually sorted.


Nit: when you say "Assert here when preds are actually sorted." it sounds like you want an assert to fire if they are sorted. I would change it to say "Enable this assert when preds are actually sorted."

CarolEidt · 2017-08-11T22:00:34Z

    assert(!fgCheapPredsValid);

-    flowList* pred;
+    // Make sure the list is sorted before manipulating.


For now I would say:

// Check to see whether the list is sorted before manipulating. // Note that the predecessors are supposed to be sorted, but currently they are not consistently kept in // that state. In order to ensure that the existing state is maintained, we will check to see whether it is // sorted before we start, and then when we are done, if it was originally sorted, we will assert that it still is.

CarolEidt · 2017-08-11T22:05:46Z

+    if (*elementBeforeMovingPred)
+    {
+        *elementBeforeMovingPred = movingPred->flNext;
+    }


I wonder how often it is the case that the new block will be in the same place in the sorted list, since it is presumably often the case that we are eliminating blocks that have close bbNums. In that case, you're deleting and reinserting unnecessarily.

But since the list is singly-linked, you can't look before you. You could only avoid unlink/re-link if the replacement block is > movingPred->flBlock->bbNum and < movingPred->flNext->flBlock->bbNum (which of course would require a null check on movingPred->flNext.)

CarolEidt · 2017-08-11T22:12:23Z

+    // Set the block to the new pred.
+    movingPred->flBlock = newPred;
+
+    // If the list is empty or the item is to be inserted at the head.


I would change this comment to:

// If there are no other predecessors, make this the first (and last) element in the list.

Note that, if you checked for the simple case first (that newPred sorts in the same place as oldPred, this case would simply fall out.

CarolEidt · 2017-08-11T22:14:08Z

+
+        // If refCounts were correct coming in, but modifying incorrectly
+        // going out assert.
+        if (refCountCorrect)


Nit: You could add this check above and avoid doing this if we already know it wasn't correct coming in.

BruceForstall · 2017-08-11T22:38:47Z

+//------------------------------------------------------------------------
+// fgExpensiveSortedPredCheck: Assert that the pred list for all Basic Blocks is sorted
+//
+// Assumptions:


Grammar ("make sense" => "makes sense", remove "when"). Suggestion:

// This check is not applicable to the cheap preds lists.

BruceForstall · 2017-08-11T22:39:14Z

+//
+// Assumptions:
+//    -- This only make sense to check when with the full predecessor lists, not the cheap preds lists.
+//


Make this a TODO-Bug:

BruceForstall · 2017-08-11T22:40:39Z

+        bool sorted = true;
+
+        for (BasicBlock* block = fgFirstBB; block != nullptr; block = block->bbNext)
+        {


Needs to be if (!fgExpensiveSortedPredCheck(block)) return false;.

Currently, it only checks the first block.

BruceForstall · 2017-08-11T22:41:15Z

+    return true;
+}
+
+//------------------------------------------------------------------------


Comment is wrong: is function checks one block, not all

BruceForstall · 2017-08-11T22:41:25Z

+//------------------------------------------------------------------------
+// fgExpensiveSortedPredCheck: Assert that the pred list for all Basic Blocks is sorted
+//
+// Assumptions:


Same comment as above

BruceForstall · 2017-08-11T22:42:48Z

    noway_assert(newPred != nullptr);
    assert(!fgCheapPredsValid);

-    flowList* pred;


Since you are making so many changes to this function, how about replacing the function header comment with one matching the current standard format.

BruceForstall · 2017-08-11T22:44:19Z

-        if (oldPred == pred->flBlock)
+        unsigned refCount = 0;
+
+        // Make sure the ref count to this block is still correct.


This comment doesn't make sense here

BruceForstall · 2017-08-11T22:46:31Z

+#if DEBUG
+    // Also make sure that our refCount is correct before modifying.
+    // If it is not, keep track of that so that we know replacePreds is
+    // not the problem.


I presume there are cases currently where the ref count is not correct?

There should be a TODO of some kind here to either add code like assert(fgRefCountMatchesPredRefCount(block)), or similar. I would hope either this whole DEBUG clause goes away, or gets replaced by a function call / assert.

BruceForstall · 2017-08-11T22:47:19Z

+    // and change this to an assert.
+    //
+    // Tracked with https://github.com/dotnet/coreclr/issues/13345
+    if (!movingPred)


CC: if (!movingPred) => if (movingPred == nullptr)

BruceForstall · 2017-08-11T22:57:05Z

+        return;
+
+    // Splice out the node that we are modifying.
+    if (*elementBeforeMovingPred)


You should not have this if; if movingPred != nullptr, then elementBeforeMovingPred is correctly set.

BruceForstall · 2017-08-11T22:58:53Z

+
+#endif // DEBUG
+
+    flowList** elementBeforeMovingPred;


nit: I wouldn't call this "elementBeforeMovingPred" because "element" might be construed to be a predecessor, which it is not. I would call it "ptrToPred", or "ptrToMovingPred".

BruceForstall · 2017-08-11T23:03:55Z

+    // If the list is empty or the item is to be inserted at the head.
+    if (block->bbPreds == nullptr)
+    {
+        // We are inserting the newPred at the end of the list.AddArgumentToTail


This comment doesn't make sense here.

BruceForstall · 2017-08-11T23:05:32Z

    noway_assert(newPred != nullptr);
    assert(!fgCheapPredsValid);

-    flowList* pred;


Should we also have:

assert(oldPred != newPred);

?

That makes sense to me.

BruceForstall · 2017-08-11T23:06:35Z

+    // Set the block to the new pred.
+    movingPred->flBlock = newPred;
+
+    // If the list is empty or the item is to be inserted at the head.


There are a bunch of cases coming up; it would be useful to have a comment at the top that enumerates them, and says how each will be handled.

BruceForstall · 2017-08-11T23:25:30Z

+            // back in the pred list.
+
+            unsigned largerCount = pred->flDupCount;
+            largerCount          = movingPred->flDupCount > largerCount ? movingPred->flDupCount : largerCount;


Doesn't the flDupCount need to be added between the existing and new preds?

BruceForstall · 2017-08-11T23:29:04Z

+    {
+        prevPred->flNext   = movingPred;
+        movingPred->flNext = nullptr;
+    }


I think all the code above could be more simply written as:

flowList** ptrToPred = &block->bbPreds; for(;;) { if ((*ptrToPred == nullptr) || ((*ptrToPred)->flBlock->bbNum > newPred->bbNum)) { // Insert after *ptrToPred (might be at end). movingPred->flNext = *ptrToPred; *ptrToPred = movingPred; break; } else if ((*ptrToPred)->flBlock->bbNum == newPred->bbNum) { // The replacement pred block is already in the list! Add the ref counts together. (*ptrToPred)->flDupCount += movingPred->flDupCount; break; } ptrToPred = &((*ptrToPred)->flNext); } fgExpensiveSortedPredCheck(block);

AndyAyersMS · 2017-08-12T14:32:33Z

@jashook Can you add a short writeup on why it's desirable to keep the preds sorted? Perhaps open an issue that talks about the benefits and costs?

BruceForstall · 2017-08-12T17:39:10Z

@AndyAyersMS That would be useful, but, IMO, is out of scope for this fix.

The only "documentation" on this requirement is, I believe, the following in fgAddRefPred:

coreclr/src/jit/flowgraph.cpp

Lines 1048 to 1056 in 8c8523c

    
           // Keep the predecessor list in lowest to highest bbNum order. This allows us to discover the loops in 
        
           // optFindNaturalLoops from innermost to outermost. 
        
           // 
        
           // TODO-Throughput: Inserting an edge for a block in sorted order requires searching every existing edge. 
        
           // Thus, inserting all the edges for a block is quadratic in the number of edges. We need to either 
        
           // not bother sorting for debuggable code, or sort in optFindNaturalLoops, or better, make the code in 
        
           // optFindNaturalLoops not depend on order. This also requires ensuring that nobody else has taken a 
        
           // dependency on this order. Note also that we don't allow duplicates in the list; we maintain a flDupCount 
        
           // count of duplication. This also necessitates walking the flow list for every edge we add.

quoting:

    // Keep the predecessor list in lowest to highest bbNum order. This allows us to discover the loops in
    // optFindNaturalLoops from innermost to outermost.
    //
    // TODO-Throughput: Inserting an edge for a block in sorted order requires searching every existing edge.
    // Thus, inserting all the edges for a block is quadratic in the number of edges. We need to either
    // not bother sorting for debuggable code, or sort in optFindNaturalLoops, or better, make the code in
    // optFindNaturalLoops not depend on order. This also requires ensuring that nobody else has taken a
    // dependency on this order. Note also that we don't allow duplicates in the list; we maintain a flDupCount
    // count of duplication. This also necessitates walking the flow list for every edge we add.

The cost/complexity of this is the reason the "cheap preds" list was invented, for early use.

I haven't investigated if this comment is still valid (namely, does optFindNaturalLoops really depend on this sorting, or are there other users who do). The dup count doesn't seem to be widely used, and could probably easily be removed by allowing dups in the preds list. That would require walking the entire list looking for dups instead of "early out" when you find the block you're looking for.

AndyAyersMS · 2017-08-14T14:29:41Z

I actually didn't intend to ask quite so generally.

What I meant to ask was, what motivated this particular bit of work -- did we find a bug, or were we trying to add a checker, did somebody just happen to notice preds were not always sorted, ...?

It is plausible loop recognition relies on sorted preds, since it is triggered by lexically backwards branches. Not clear if this is a hard requirement since given two loops with the same "first" there are likely cheap checks to decide which one is the inner loop (compare lexical extents, etc).

jashook · 2017-08-14T16:30:07Z

This work was motivated by a bug found by jitstress. Because removing a pred relies on a sorted pred list. There was an assert fired that a block (although removed) still had a pred in another node's pred list. This happened because removing the pred did not take into account possible duplicates or that the pred list is currently unsorted when assumed to be sorted.

JosephTremoulet · 2017-08-22T16:49:44Z

Quoting @AndyAyersMS over in #13314 (comment),

the old code ... allowed it as long as entry's predecessor list had an inner-loop predecessor prior to having an outer-loop predecessor (oddly). So I've fixed my code to recognize nested loops... without the bizarro restriction on predecessor list.

Yeah, this is the whole "keep the pred list sorted" thing that came up over in #13322. If loop recognition no longer needs this then maybe we can drop the whole thing.

I just wanted to confirm that yes I have removed that dependence of fgFindNaturalLoops on predecessor list order. The new code still processes predecessor edges in whatever order the list presents them, so I can't rule out that changing the order of the list could lead to diffs (nor have I experimented to see how rare such diffs would be), but I know now that any dependence on order isn't intended as a correctness constraint, and in fact that it would be preferable to use the reverse order (I just didn't think it preferable enough to spend the time sorting the list) since then we'd always compact outer loops before inner ones.

I don't know what that means for this change...

BruceForstall · 2017-08-22T22:35:38Z

It seems that as long as we maintain an edge dup count, on edge insertion we need to either search the entire edge list, or keep it sorted to allow early-out.

karelz · 2017-09-06T17:14:57Z

What is status of this PR? Looks like no activity for last 2 weeks ...

jashook · 2017-09-07T02:50:14Z

That is a good question. It is worth more discussion to determine what the fix is. It sounds like the answer is no longer maintain sorted preds, but instead remove the dependency. I will close this and open an issue to track the bug.

jashook added the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Aug 10, 2017

jashook requested review from BruceForstall and CarolEidt August 10, 2017 23:07

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

Maintain sorted preds part 1

db3cb2c

This adds asserts to enforce sorted pred lists and changes fgReplacePred to correctly maintain sorted order when replacing a pred in the list.

jashook force-pushed the assert_sorted_preds branch from d743c36 to db3cb2c Compare August 11, 2017 21:14

Apply format patch

ab49346

jashook changed the title ~~Maintain sorted preds part 1 [WIP]~~ Maintain sorted preds part 1 Aug 11, 2017

Add an GH issue for fgReplacePred.

be3f140

CarolEidt reviewed Aug 11, 2017

View reviewed changes

BruceForstall reviewed Aug 11, 2017

View reviewed changes

AndyAyersMS mentioned this pull request Aug 17, 2017

Lay out loop bodies contiguously #13314

Merged

karelz added the area-CodeGen label Aug 25, 2017

karelz assigned jashook Aug 25, 2017

RussKeldorph added this to the 2.1.0 milestone Aug 25, 2017

jashook closed this Sep 7, 2017

Uh oh!

Conversation

jashook commented Aug 10, 2017

Uh oh!

jashook commented Aug 11, 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!

AndyAyersMS commented Aug 12, 2017

Uh oh!

BruceForstall commented Aug 12, 2017

Uh oh!

AndyAyersMS commented Aug 14, 2017

Uh oh!

jashook commented Aug 14, 2017

Uh oh!

JosephTremoulet commented Aug 22, 2017

Uh oh!

BruceForstall commented Aug 22, 2017

Uh oh!

karelz commented Sep 6, 2017

Uh oh!

jashook commented Sep 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants