Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
143293003:
A64: Implement LCodeGen::GenerateDeoptJumpTable (Closed)
|
Created:
6 years, 10 months ago by jochen (gone - plz use gerrit) Modified:
6 years, 10 months ago CC:
v8-dev Visibility:
Public. |
DescriptionA64: Implement LCodeGen::GenerateDeoptJumpTable
BUG=none
R=rodolph.perfetta@arm.com, ulan@chromium.org
LOG=n
Committed: https://code.google.com/p/v8/source/detail?r=19139
Patch Set 1 #
Total comments: 6
Messages
Total messages: 9 (0 generated)
plz review https://codereview.chromium.org/143293003/diff/1/src/a64/lithium-codegen-a64.cc File src/a64/lithium-codegen-a64.cc (right): https://codereview.chromium.org/143293003/diff/1/src/a64/lithium-codegen-a64.... src/a64/lithium-codegen-a64.cc:835: bool LCodeGen::GenerateDeoptJumpTable() { ARM has a check that the table is not too far away. I didn't put the check here, because eventually, we'll support arbitrary long jumps, right?
https://codereview.chromium.org/143293003/diff/1/src/a64/lithium-codegen-a64.cc File src/a64/lithium-codegen-a64.cc (right): https://codereview.chromium.org/143293003/diff/1/src/a64/lithium-codegen-a64.... src/a64/lithium-codegen-a64.cc:840: __ bind(&table_start); Use Bind instead. https://codereview.chromium.org/143293003/diff/1/src/a64/lithium-codegen-a64.... src/a64/lithium-codegen-a64.cc:853: __ Mov(__ Tmp0(), Operand(ExternalReference::ForDeoptEntry(entry))); I am not sure that we can use Tmp0, Tmp1 here. Rodolph, when is it safe to use these registers?
https://codereview.chromium.org/143293003/diff/1/src/a64/lithium-codegen-a64.cc File src/a64/lithium-codegen-a64.cc (right): https://codereview.chromium.org/143293003/diff/1/src/a64/lithium-codegen-a64.... src/a64/lithium-codegen-a64.cc:853: __ Mov(__ Tmp0(), Operand(ExternalReference::ForDeoptEntry(entry))); On 2014/02/05 10:25:41, ulan wrote: > I am not sure that we can use Tmp0, Tmp1 here. Rodolph, when is it safe to use > these registers? Tmp1() was uses in the old code (see line 1009) so I assumed it's safe.
https://codereview.chromium.org/143293003/diff/1/src/a64/lithium-codegen-a64.cc File src/a64/lithium-codegen-a64.cc (right): https://codereview.chromium.org/143293003/diff/1/src/a64/lithium-codegen-a64.... src/a64/lithium-codegen-a64.cc:835: bool LCodeGen::GenerateDeoptJumpTable() { Right, but in the mean time a debug check may save us some time when investigating bugs. On 2014/02/05 08:57:38, jochen wrote: > ARM has a check that the table is not too far away. I didn't put the check here, > because eventually, we'll support arbitrary long jumps, right? https://codereview.chromium.org/143293003/diff/1/src/a64/lithium-codegen-a64.... src/a64/lithium-codegen-a64.cc:853: __ Mov(__ Tmp0(), Operand(ExternalReference::ForDeoptEntry(entry))); In Theory it is never safe to use them outside the macro assembler. It just so happen that instruction used here don't need tmps at all. We'll revisit their uses later but in the mean time asserting the code sequence in the else clause below has a fixed length (therefore doesn't use tmps) would be good. On 2014/02/05 10:34:17, jochen wrote: > On 2014/02/05 10:25:41, ulan wrote: > > I am not sure that we can use Tmp0, Tmp1 here. Rodolph, when is it safe to use > > these registers? > > Tmp1() was uses in the old code (see line 1009) so I assumed it's safe.
On 2014/02/05 10:52:42, Rodolph Perfetta (ARM) wrote: > https://codereview.chromium.org/143293003/diff/1/src/a64/lithium-codegen-a64.cc > File src/a64/lithium-codegen-a64.cc (right): > > https://codereview.chromium.org/143293003/diff/1/src/a64/lithium-codegen-a64.... > src/a64/lithium-codegen-a64.cc:835: bool LCodeGen::GenerateDeoptJumpTable() { > Right, but in the mean time a debug check may save us some time when > investigating bugs. The branch instructions to the jump table entries already check that they're not too far, no? > > On 2014/02/05 08:57:38, jochen wrote: > > ARM has a check that the table is not too far away. I didn't put the check > here, > > because eventually, we'll support arbitrary long jumps, right? > > https://codereview.chromium.org/143293003/diff/1/src/a64/lithium-codegen-a64.... > src/a64/lithium-codegen-a64.cc:853: __ Mov(__ Tmp0(), > Operand(ExternalReference::ForDeoptEntry(entry))); > In Theory it is never safe to use them outside the macro assembler. It just so > happen that instruction used here don't need tmps at all. We'll revisit their > uses later but in the mean time asserting the code sequence in the else clause > below has a fixed length (therefore doesn't use tmps) would be good. > > On 2014/02/05 10:34:17, jochen wrote: > > On 2014/02/05 10:25:41, ulan wrote: > > > I am not sure that we can use Tmp0, Tmp1 here. Rodolph, when is it safe to > use > > > these registers? > > > > Tmp1() was uses in the old code (see line 1009) so I assumed it's safe.
On 2014/02/06 09:42:17, jochen wrote: > On 2014/02/05 10:52:42, Rodolph Perfetta (ARM) wrote: > > > https://codereview.chromium.org/143293003/diff/1/src/a64/lithium-codegen-a64.cc > > File src/a64/lithium-codegen-a64.cc (right): > > > > > https://codereview.chromium.org/143293003/diff/1/src/a64/lithium-codegen-a64.... > > src/a64/lithium-codegen-a64.cc:835: bool LCodeGen::GenerateDeoptJumpTable() { > > Right, but in the mean time a debug check may save us some time when > > investigating bugs. > > The branch instructions to the jump table entries already check that they're not > too far, no? Yep good point :-)
so, ok to land?
LGTM. Can you add a todo to revisit the use of Tmpx() in the table code.
Message was sent while issue was closed.
Committed patchset #1 manually as r19139 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
