Enable globopt for functions with try finally - #2954

meg-gupta · 2017-05-10T10:12:33Z

This change enables globopt on functions with try finally.
We transform the flowgraph such that we have 2 flow edges -
try to a excepting finally region and try to non excepting finally region.
This enables us to optimize the function on the non exception path.
We bailout on the exception path.

Special handling is needed when there are early exits (break, continue, return) within the tryfinally.
We need to execute the finally block on early exit, currently we bailout on early exit.
We transform the flow graph from (eh region -> early exit) to have an edge from (eh region -> finally) and (finally -> early exit)
This transformation can be done only after the regions are assigned in FlowGraph.

So the flowgraph builder now has the following order of phase:

build flow graph -> add the excepting and non excepting finallys alongside
remove unreachable blocks
assign regions
identify early exits and add edges (does region info update when required)
break blocks removal (does region info update when required)

rajatd · 2017-05-11T00:20:26Z

-
-            if (this->catchOffsetStack && !this->catchOffsetStack->Empty())
+            if ((this->handlerOffsetStack && !this->handlerOffsetStack->Empty()) ||
+                inFinallyBlock > 0)


inFinallyBlock [](start = 16, length = 14)

inFinallyBlock [](start = 16, length = 14)

nit: for better readability, change name to finallyBlockLevel or something or have an API InFinallyBlock. #Resolved

meg-gupta · 2017-05-12T17:34:15Z

@dotnet-bot test OSX static_osx_osx_debug

rajatd · 2017-05-15T16:09:00Z

        this->catchLabelStack = JitAnew(this->alloc, SList<IR::LabelInstr*>, this->alloc);
    }
+    if (this->func->HasFinally() && (this->func->DoOptimizeTryFinally() ||
+        (this->func->IsSimpleJit() && this->func->GetJITFunctionBody()->DoJITLoopBody())))


this->func->IsSimpleJit() && this->func->GetJITFunctionBody()->DoJITLoopBody()))) [](start = 9, length = 81)

Take this condition out and check for DoOptimizeTryCatch and DoOptimizeTryFinally inside it. Actually, this makes me think - we can't turn off the opitimizer for try-catch and have it on for try-finally, or vice-versa, right?

Oh, I was looking at the 2nd iteration - second issue seems fixed in 3rd iteration :). We should still hoist the common conditions though

In reply to: 116533348 [](ancestors = 116533348)

Will do. thanks. We can turn off try finally opt without affecting try catch opt, but if we turn off try catch opt, try finally is turned off too.

"We can turn off try finally opt without affecting try catch opt" - how? If we're not optimizing try-finally but are try-catch, and the trycatch is inside try-finally, wouldn't that be a problem?

In reply to: 116535802 [](ancestors = 116535802)

rajatd · 2017-05-15T16:48:12Z

            }
+            if (instr->IsLabelInstr() && instr->AsLabelInstr()->m_isLoopTop &&
+                this->finallyLabelStack && !this->finallyLabelStack->Empty() &&
+                instr->m_next->m_opcode != Js::OpCode::BrOnException)


hoist the common conditions #Resolved

rajatd · 2017-05-15T16:48:22Z

+            // LeaveNull is needed to get the end of the finally block, this is important when adding edge from the finally block to the early exit block
+            if (instr->IsLabelInstr() && instr->AsLabelInstr()->m_isLoopTop &&
+                this->leaveNullLabelStack && !this->leaveNullLabelStack->Empty() &&
+                instr->m_next->m_opcode != Js::OpCode::BrOnException)


same #Resolved

rajatd · 2017-05-15T16:49:30Z

+            // Create a new label before LeaveNull
+            IR::LabelInstr *leaveNullLabel = IR::LabelInstr::New(Js::OpCode::Label, this->func);
+            instr->InsertBefore(leaveNullLabel);
+            this->leaveNullLabelStack->Push(leaveNullLabel);


can there be multiple LeaveNulls for a finally? #Resolved

No, only one at the end of the finally #Resolved

then it might be worth asserting after the flowgraph is built that the leaveNullLabelStack is null or empty (similar to catchLabelStack and finallyLabelStack) #Resolved

rajatd · 2017-05-15T16:50:00Z

+
    FOREACH_BLOCK_ALL(block, this)
    {
        block->SetBlockNum(blockNum++);


can you do this in the following loop? #Resolved

rajatd · 2017-05-15T16:53:44Z

            }
        }
+
+        if (instr->IsLabelInstr() && instr->m_next->m_opcode == Js::OpCode::Finally)


Most of the code in this block is the same as in the previous one. Please refactor into one if block. #Resolved

rajatd · 2017-05-15T16:57:01Z

-        ProcessTryCatch(instr);
+        ProcessTryCatchOrTryFinally(instr);
+    }
+    else if (instr->m_opcode == Js::OpCode::TryFinally && this->func->DoOptimizeTry())


Please merge into the if block above with doing an || of the opcodes condition #Resolved

rajatd · 2017-05-15T16:58:44Z

+            Region *targetRegion = instr->AsBranchInstr()->GetTarget()->GetRegion();
+            Assert((targetRegion->GetType() == RegionType::RegionTypeFinally &&
+                targetRegion->GetMatchingTryRegion()->GetMatchingFinallyRegion(true) == targetRegion) ||
+                true);


'true' left over? #Resolved

rajatd · 2017-05-15T18:15:21Z

    IR::Instr*      LowerTry(IR::Instr* instr, bool tryCatch);
    void            EnsureBailoutReturnValueSym();
    void            EnsureHasBailedOutSym();
+    Region *        GetNonNonExceptingFinallyGrandParent(Region *region);


nit: double 'non' #Resolved

also, this should be on the Region class #Resolved

And rename to GetFirstNonExceptingAncestor, and refactor code in Lowerer::InsertReturnThunkForRegion as discussed offline

rajatd · 2017-05-15T19:22:11Z


        IR::LabelOpnd * continuationAddr;
-        if (region->GetParent()->GetType() != RegionTypeRoot)
+        if (region->IsNonExceptingFinally())


Add lots of comments and asserts to explain the mechanics here and to enforce your invariants

rajatd · 2017-05-15T21:09:00Z

            exception = err.GetAndClear();
        }

+        if (exception)


you need to check for hasBailedOutOffset in this function and also generate code in LowerMD.cpp to pass the param to this helper #Resolved

rajatd · 2017-05-16T00:02:25Z

+            BasicBlock * nextBlock = nextLabel->GetBasicBlock();
+            IR::BranchInstr * branchTofinallyBlockOrEarlyExit = nextLabel->m_next->AsBranchInstr();
+            IR::LabelInstr * finallyBlockLabelOrEarlyExitLabel = branchTofinallyBlockOrEarlyExit->GetTarget();
+            if (finallyBlockLabelOrEarlyExitLabel->GetRegion() == this->currentRegion)


Did not understand this condition

rajatd · 2017-05-16T00:27:31Z

+            //          TryFinally L1
+            //          <try code>
+            //          BrOnException L1
+            //          Leave L1'


It appears to me that L1' is really L2 from before the transformation. If so, lets keep L2 only after the transformation. Makes understanding the code below easier #Resolved

rajatd · 2017-05-16T01:41:55Z

+
+    while (leaveChain->m_opcode != Js::OpCode::Br)
+    {
+        IR::LabelInstr * nextLabel = leaveChain->m_next->AsLabelInstr();


leaveChain->m_next guaranteed to always be a labelInstr?

rajatd · 2017-05-16T02:04:00Z

+
+        IR::LabelInstr * currentLabel = nullptr;
+        // look for early exits from a try, and insert bailout
+        FOREACH_INSTR_IN_FUNC_EDITING(instr, instrPrev, func)


please call it instrNext #Resolved

rajatd · 2017-05-16T06:10:58Z

+                    branchTargetRegion != currentRegion->GetMatchingFinallyRegion(true));
+            if (!isEarly) return false;
+
+            if (DoesExitLabelDominate(leaveInstr)) return false;


we could probably cache the result of this call before region condition checks

rajatd · 2017-05-16T06:25:41Z

Looked at most of the changes and gave some comments. Flowgraph changes are definitely complicated. Looks good overall.

meg-gupta · 2017-05-16T20:04:19Z

@dotnet-bot test Ubuntu shared_ubuntu_linux_test
@dotnet-bot test Ubuntu shared_ubuntu_linux_debug
@dotnet-bot test OSX static_osx_osx_debug

rajatd · 2017-05-17T00:18:33Z

Oh, and yes, add asserts for opcodes, for example, at places you are looping while the instruction is not a Br, and it seems that you expect the instr to be Leave.

This change enables globopt on functions with try finally. We transform the flowgraph such that we have 2 flow edges - try to a excepting finally region and try to non excepting finally region. This enables us to optimize the function on the non exception path. We bailout on the exception path. Special handling is needed when there are early exits (break, continue, return) within the tryfinally. We need to execute the finally block on early exit, currently we bailout on early exit. We transform the flow graph from (eh region -> early exit) to have an edge from (eh region -> finally) and (finally -> early exit) This transformation can be done only after the regions are assigned in FlowGraph. So the flowgraph builder now has the following order of phase: - build flow graph -> add the excepting and non excepting finallys alongside - remove unreachable blocks - assign regions - identify early exits and add edges (does region info update when required) - break blocks removal (does region info update when required)

Merge pull request #2954 from meg-gupta:tryfinallypr This change enables globopt on functions with try finally. We transform the flowgraph such that we have 2 flow edges - try to a excepting finally region and try to non excepting finally region. This enables us to optimize the function on the non exception path. We bailout on the exception path. Special handling is needed when there are early exits (break, continue, return) within the tryfinally. We need to execute the finally block on early exit, currently we bailout on early exit. We transform the flow graph from (eh region -> early exit) to have an edge from (eh region -> finally) and (finally -> early exit) This transformation can be done only after the regions are assigned in FlowGraph. So the flowgraph builder now has the following order of phase: - build flow graph -> add the excepting and non excepting finallys alongside - remove unreachable blocks - assign regions - identify early exits and add edges (does region info update when required) - break blocks removal (does region info update when required)

msftclas added the cla-not-required label May 10, 2017

meg-gupta force-pushed the tryfinallypr branch 3 times, most recently from 67a652d to ea97909 Compare May 10, 2017 13:53

meg-gupta requested review from LouisLaf, pleath and rajatd May 10, 2017 13:59

rajatd reviewed May 11, 2017

View reviewed changes

meg-gupta force-pushed the tryfinallypr branch 4 times, most recently from e292898 to bc15c38 Compare May 12, 2017 03:05

rajatd reviewed May 15, 2017

View reviewed changes

rajatd reviewed May 16, 2017

View reviewed changes

meg-gupta force-pushed the tryfinallypr branch 3 times, most recently from 9f6f38b to 020b17f Compare May 16, 2017 19:41

meg-gupta force-pushed the tryfinallypr branch from 020b17f to 606210f Compare May 16, 2017 23:56

meg-gupta force-pushed the tryfinallypr branch from 606210f to 4756472 Compare May 17, 2017 00:29

meg-gupta force-pushed the tryfinallypr branch from 4756472 to 82ff317 Compare May 17, 2017 00:58

chakrabot merged commit 82ff317 into chakra-core:master May 17, 2017

Uh oh!

Conversation

meg-gupta commented May 10, 2017

Uh oh!

rajatd May 11, 2017 • edited by meg-gupta Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meg-gupta commented May 12, 2017

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!

rajatd May 15, 2017 • edited by meg-gupta Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajatd May 15, 2017 • edited by meg-gupta Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajatd May 15, 2017 • edited by meg-gupta Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meg-gupta May 15, 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!

rajatd May 15, 2017 • edited by meg-gupta Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajatd May 15, 2017 • edited by meg-gupta Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajatd May 15, 2017 • edited by meg-gupta Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajatd May 15, 2017 • edited by meg-gupta Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajatd May 15, 2017 • edited by meg-gupta Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajatd May 15, 2017 • edited by meg-gupta Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajatd May 15, 2017 • edited by meg-gupta 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!

rajatd May 15, 2017 • edited by meg-gupta 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!

rajatd May 16, 2017 • edited by meg-gupta 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!

rajatd May 16, 2017 • edited by meg-gupta Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

rajatd May 11, 2017 •

edited by meg-gupta

Loading

rajatd May 15, 2017 •

edited by meg-gupta

Loading

rajatd May 15, 2017 •

edited by meg-gupta

Loading

rajatd May 15, 2017 •

edited by meg-gupta

Loading

meg-gupta May 15, 2017 •

edited

Loading

rajatd May 15, 2017 •

edited by meg-gupta

Loading

rajatd May 15, 2017 •

edited by meg-gupta

Loading

rajatd May 15, 2017 •

edited by meg-gupta

Loading

rajatd May 15, 2017 •

edited by meg-gupta

Loading

rajatd May 15, 2017 •

edited by meg-gupta

Loading

rajatd May 15, 2017 •

edited by meg-gupta

Loading

rajatd May 15, 2017 •

edited by meg-gupta

Loading

rajatd May 15, 2017 •

edited by meg-gupta

Loading

rajatd May 16, 2017 •

edited by meg-gupta

Loading

rajatd May 16, 2017 •

edited by meg-gupta

Loading