Commit ffffed90 authored by Pierre Langlois's avatar Pierre Langlois Committed by Commit Bot

[arm64] Fix backwards branch ranges.

The `Instruction::IsValidImmPCOffset()` method was taking an `offset` argument
in numbers of *instructions* while we were passing it numbers of *bytes*. See
`Instruction::IsTargetInImmPCOffsetRange()` and
`MacroAssembler::NeedExtraInstructionsOrRegisterBranch()`.

As a result, we were 4 times too conservative when computing branch ranges going
backwards, forcing us to generate the following sequence for TBZ more often than
needed:

```
  TBNZ <skip>
  B <target>
skip:
```

This happened rarely for loops, but a lot when doing an early return from
out-of-line calls to write barriers. Since out-of-line code is easily out of
range of 8K, although the real range of TBZ is 32K.

This fixes it by changing this method to take a byte offset instead of
instructions, as this is more intuitive and in line with similar methods. For
instance, `Instruction::ImmPcOffset()` returns an offset in bytes.

The tests are adapted so that they would have caught such a bug:

* TEST(far_branch_backward):

  This test used to only check the code worked if the branch was very far away,
  but it didn't test the range was correct. So this test was changed to check
  each branch type separately, and test in-range and out-of-range cases
  separately too.

* TEST(far_branch_veneer_broken_link_chain):

  Because of the backwards range bug, this test wasn't actually testing what it
  should. The idea of the test is to make sure the MacroAssembler can still cope
  when the chain of links is broken after a veneer was emitted. But no veneers
  were ever emitted.

Change-Id: Iddb5c683a71147455175f38fa7ae57da0a3e7337
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1781058Reviewed-by: 's avatarUlan Degenbaev <ulan@chromium.org>
Reviewed-by: 's avatarSigurd Schneider <sigurds@chromium.org>
Commit-Queue: Sigurd Schneider <sigurds@chromium.org>
Cr-Commit-Position: refs/heads/master@{#63518}
parent f3796bbc
......@@ -211,7 +211,8 @@ Instruction* Instruction::ImmPCOffsetTarget() {
bool Instruction::IsValidImmPCOffset(ImmBranchType branch_type,
ptrdiff_t offset) {
return is_intn(offset, ImmBranchRangeBitwidth(branch_type));
DCHECK_EQ(offset % kInstrSize, 0);
return is_intn(offset / kInstrSize, ImmBranchRangeBitwidth(branch_type));
}
bool Instruction::IsTargetInImmPCOffsetRange(Instruction* target) {
......@@ -251,8 +252,7 @@ void Instruction::SetPCRelImmTarget(const AssemblerOptions& options,
void Instruction::SetBranchImmTarget(Instruction* target) {
DCHECK(IsAligned(DistanceTo(target), kInstrSize));
DCHECK(
IsValidImmPCOffset(BranchType(), DistanceTo(target) >> kInstrSizeLog2));
DCHECK(IsValidImmPCOffset(BranchType(), DistanceTo(target)));
int offset = static_cast<int>(DistanceTo(target) >> kInstrSizeLog2);
Instr branch_imm = 0;
uint32_t imm_mask = 0;
......
......@@ -390,6 +390,8 @@ class Instruction {
// PC-relative addressing instruction.
V8_EXPORT_PRIVATE Instruction* ImmPCOffsetTarget();
// Check if the offset is in range of a given branch type. The offset is
// a byte offset, unscaled.
static bool IsValidImmPCOffset(ImmBranchType branch_type, ptrdiff_t offset);
bool IsTargetInImmPCOffsetRange(Instruction* target);
// Patch a PC-relative offset to refer to 'target'. 'this' may be a branch or
......
This diff is collapsed.
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment