https://codereview.chromium.org/169893002/diff/1/src/a64/assembler-a64.cc
File src/a64/assembler-a64.cc (right):
https://codereview.chromium.org/169893002/diff/1/src/a64/assembler-a64.cc#newcode388
src/a64/assembler-a64.cc:388: // The branch is the first instruction in
the chain.
On 2014/02/18 10:17:40, ulan wrote:
I think these comments do not agree with the start_of_chain definition
above.
Are instructions counted from the start of the chain? If yes, then
shouldn't it
be:
(branch == prev_link) => branch is the last
(branch == next_link) => branch is the first
Renaming `start_of_chain` to `end_of_chain` here and in
Assembler::CheckLabelLinkChain().
https://codereview.chromium.org/169893002/diff/1/src/a64/macro-assembler-a64.cc
File src/a64/macro-assembler-a64.cc (right):
https://codereview.chromium.org/169893002/diff/1/src/a64/macro-assembler-a64.cc#newcode635
src/a64/macro-assembler-a64.cc:635: margin += margin / 4;
On 2014/02/18 10:17:40, ulan wrote:
Where 4 comes from?
Complete finger in the air guess.
With a default margin of 2KB that leaves us 512B = 128 instructions of
extra margin to avoid requiring a protective branch. I thought this
sounded about right.
Added a comment.
https://codereview.chromium.org/169893002/diff/1/src/a64/macro-assembler-a64.cc#newcode659
src/a64/macro-assembler-a64.cc:659: FarBranchInfo(pc_offset(), label)));
On 2014/02/18 10:17:40, ulan wrote:
If the CheckVeneers below emits veneers then the pc_offset() will no
longer
point to the current branch instruction.
Ouch. Thanks.
Maybe move CheckVeneers outside of this function?
Done.
https://codereview.chromium.org/169893002/diff/1/src/a64/macro-assembler-a64.h
File src/a64/macro-assembler-a64.h (right):
https://codereview.chromium.org/169893002/diff/1/src/a64/macro-assembler-a64.h#newcode2123
src/a64/macro-assembler-a64.h:2123: static const int
kVeneerDistanceMargin = 2 * KB;
On 2014/02/18 10:17:40, ulan wrote:
To check my understanding:
if there are no branch and ret instructions in
[unresolved_branches_first_limit() - 2 * KB - (guard + veneer size) ..
unresolved_branches_first_limit()] then at least one branch will be
broken?
Yes. This would fire a CHECK.
My take was that we generate branches often enough (unluckily) that this
was safe.
Now I think it would be better to use a `next_veneer_check_` offset like
we do for constant pools. However that would require moving some veneer
code to the Assembler (while we want to keep this in the MAsm).
Rodolph is considering moving the relocation to the MAsm (separate
issue) and improving CheckBuffer(), so I think for now we can log an
issue on our side (to avoid a TODO in the code) and do this a bit later.
The current situation should still be safe.
https://codereview.chromium.org/169893002/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
