Refactor interrupt check patching for OSR.

Refactor interrupt check patching for OSR.

This is to prepare for speculative concurrent OSR. I'm planning to add
another builtin to patch to, to indicate a concurrent OSR.

R=titzer@chromium.org
BUG=

Committed: https://code.google.com/p/v8/source/detail?r=16425

[titzer, 2013-08-28 12:32:55]

I really like this cleanup, just a couple of minor questions.

https://codereview.chromium.org/23608004/diff/1/src/deoptimizer.cc
File src/deoptimizer.cc (right):

https://codereview.chromium.org/23608004/diff/1/src/deoptimizer.cc#newcode2587
src/deoptimizer.cc:2587: if (!interrupt_stub.FindCodeInCache(&interrupt_code,
isolate)) return;
Should this be an error?

https://codereview.chromium.org/23608004/diff/1/src/deoptimizer.cc#newcode2630
src/deoptimizer.cc:2630: ASSERT_NE(NOT_PATCHED, GetInterruptPatchState(isolate,
ASSERT_EQ(PATCHED_FOR_OSR, ... ?

https://codereview.chromium.org/23608004/diff/1/src/deoptimizer.h
File src/deoptimizer.h (right):

https://codereview.chromium.org/23608004/diff/1/src/deoptimizer.h#newcode265
src/deoptimizer.h:265: static void VerifyInterruptCode(Isolate* isolate,
Maybe this should take a patch state to check against?

[Yang, 2013-08-28 12:41:57]

I deliberately made some asserts not as strict as they could be, assuming that
we would introduce a second builtin to patch to. Since we are not going to do
that anytime soon, I'll tighten the screws.

https://codereview.chromium.org/23608004/diff/1/src/deoptimizer.cc
File src/deoptimizer.cc (right):

https://codereview.chromium.org/23608004/diff/1/src/deoptimizer.cc#newcode2587
src/deoptimizer.cc:2587: if (!interrupt_stub.FindCodeInCache(&interrupt_code,
isolate)) return;
On 2013/08/28 12:32:55, titzer wrote:
> Should this be an error?

yup. I'll turn that into an UNREACHABLE.

https://codereview.chromium.org/23608004/diff/1/src/deoptimizer.cc#newcode2630
src/deoptimizer.cc:2630: ASSERT_NE(NOT_PATCHED, GetInterruptPatchState(isolate,
On 2013/08/28 12:32:55, titzer wrote:
> ASSERT_EQ(PATCHED_FOR_OSR, ... ?

In this particular case, ASSERT_EQ(PATCHED_FOR_OSR, ...) is the same as
ASSERT_NE(NOT_PATCHED, ...), but in the case we introduce a new builtin, the
latter is better.

https://codereview.chromium.org/23608004/diff/1/src/deoptimizer.h
File src/deoptimizer.h (right):

https://codereview.chromium.org/23608004/diff/1/src/deoptimizer.h#newcode265
src/deoptimizer.h:265: static void VerifyInterruptCode(Isolate* isolate,
On 2013/08/28 12:32:55, titzer wrote:
> Maybe this should take a patch state to check against?

Again, this was with the assumption that we want to introduce a second builtin
to patch to. In that case, we would have a mixed patch state to revert from.

[Yang, 2013-08-28 12:48:52]

On 2013/08/28 12:41:57, Yang wrote:
> I deliberately made some asserts not as strict as they could be, assuming that
> we would introduce a second builtin to patch to. Since we are not going to do
> that anytime soon, I'll tighten the screws.
> 
> https://codereview.chromium.org/23608004/diff/1/src/deoptimizer.cc
> File src/deoptimizer.cc (right):
> 
> https://codereview.chromium.org/23608004/diff/1/src/deoptimizer.cc#newcode2587
> src/deoptimizer.cc:2587: if (!interrupt_stub.FindCodeInCache(&interrupt_code,
> isolate)) return;
> On 2013/08/28 12:32:55, titzer wrote:
> > Should this be an error?
> 
> yup. I'll turn that into an UNREACHABLE.
> 
> https://codereview.chromium.org/23608004/diff/1/src/deoptimizer.cc#newcode2630
> src/deoptimizer.cc:2630: ASSERT_NE(NOT_PATCHED,
GetInterruptPatchState(isolate,
> On 2013/08/28 12:32:55, titzer wrote:
> > ASSERT_EQ(PATCHED_FOR_OSR, ... ?
> 
> In this particular case, ASSERT_EQ(PATCHED_FOR_OSR, ...) is the same as
> ASSERT_NE(NOT_PATCHED, ...), but in the case we introduce a new builtin, the
> latter is better.
> 
> https://codereview.chromium.org/23608004/diff/1/src/deoptimizer.h
> File src/deoptimizer.h (right):
> 
> https://codereview.chromium.org/23608004/diff/1/src/deoptimizer.h#newcode265
> src/deoptimizer.h:265: static void VerifyInterruptCode(Isolate* isolate,
> On 2013/08/28 12:32:55, titzer wrote:
> > Maybe this should take a patch state to check against?
> 
> Again, this was with the assumption that we want to introduce a second builtin
> to patch to. In that case, we would have a mixed patch state to revert from.

Another comment to VerifyInterruptCode: it's supposed to check that patch sites
below a certain loop nesting level are patched. Currently, to express that all
patch sites are unpatched, we simply pass -1 as loop nesting level. There is no
case were we want to check that patch sites below a certain loop nesting level
are unpatched and above are patched, so I don't think passing a patch state is
necessary.

[titzer, 2013-08-29 11:57:18]

lgtm

[Yang, 2013-08-29 13:06:17]

Committed patchset #2 manually as r16425 (presubmit successful).