Page MenuHomePhabricator

[AArch64InstrInfo] Stop getInstSizeInBytes returning non-zero for meta instructions.
ClosedPublic

Authored by paulwalker-arm on Aug 16 2019, 2:57 AM.

Details

Summary

AArch64InstrInfo::getInstSizeInBytes is incorrectly treating meta
instructions (e.g. CFI_INSTRUCTION) as normal instructions and
giving them a size of 4.

This results in branch relaxation calculating block sizes wrong.
Branch relaxation also considers alignment and thus a single
mistake can result in later blocks being incorrectly sized even
when they themselves do not contain meta instructions.

The net result is we might not relax a branch whose destination is
not within range.

Diff Detail

Repository
rL LLVM

Event Timeline

paulwalker-arm created this revision.Aug 16 2019, 2:57 AM
peter.smith accepted this revision.Aug 16 2019, 3:54 AM

Looks good to me.

Cross checking against ARM to see if the same problem exists there, it looks like a different approach is taken, where anything unrecognized is assumed to be 0 size. This would handle meta instructions, but could underestimate block size if it came across something it didn't recognise. So I think this approach works best for AArch64.

The net result is we might not relax a branch whose destination is not within range.

I'm guessing you mean, without this change we might not relax a branch whose destination is within range.

This revision is now accepted and ready to land.Aug 16 2019, 3:54 AM

Have I got the terminology the wrong way round? I assumed relaxing meant the act of converting a branch who destination is not within range into a one that is within range either by inversion or introducing a second branch with sufficient range. I just want to make sure my commit message makes sense.

Have I got the terminology the wrong way round? I assumed relaxing meant the act of converting a branch who destination is not within range into a one that is within range either by inversion or introducing a second branch with sufficient range. I just want to make sure my commit message makes sense.

Sorry, I think it is me that has it wrong. I'm used to Linkers where relaxation can mean replace something like replace an expensive GlobalDynamic TLS sequence with a cheaper LocalDynamic one. In LLVM the branch relaxation pass starts with a small branch and replaces with a larger one if the smaller one is out of range so I think it is better to stick to your original one.

This revision was automatically updated to reflect the committed changes.

This test seems to be failing on our mac builders:

Testing: 0 .. 10..
FAIL: LLVM :: CodeGen/AArch64/branch-relax-block-size.mir (6095 of 33081)
******************** TEST 'LLVM :: CodeGen/AArch64/branch-relax-block-size.mir' FAILED ********************
Script:
--
: 'RUN: at line 2';   /b/s/w/ir/k/recipe_cleanup/clangOx7vi0/llvm_build_dir/bin/llc -mtriple=aarch64--linux-gnu -run-pass=branch-relaxation -debug-only=branch-relaxation /b/s/w/ir/k/llvm-project/llvm/test/CodeGen/AArch64/branch-relax-block-size.mir -o /dev/null 2>&1 | /b/s/w/ir/k/recipe_cleanup/clangOx7vi0/llvm_build_dir/bin/FileCheck /b/s/w/ir/k/llvm-project/llvm/test/CodeGen/AArch64/branch-relax-block-size.mir
--
Exit Code: 1

Command Output (stderr):
--
/b/s/w/ir/k/llvm-project/llvm/test/CodeGen/AArch64/branch-relax-block-size.mir:8:15: error: CHECK-NEXT: expected string not found in input
# CHECK-NEXT: %bb.0 offset=00000000 size=0x18
              ^
<stdin>:3:1: note: scanning from here
bb.0 offset=00000000 size=0x18
^

--

********************
Testing: 0 .. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90.. 
Testing Time: 119.28s
********************
Failing Tests (1):
    LLVM :: CodeGen/AArch64/branch-relax-block-size.mir

Seems to just be failing from a missing %.

Thanks for the report, I've committed a version without the %s that will hopefully fix the issue.

I've committed a better fix in r369138.

<stdin>:3:1: note: non-matching line after previous match is here

215234==AddressSanitizer: WARNING: unexpected format specifier in printf interceptor: %b (reported once per process)

I've committed a better fix in r369138.

Many Thanks. As I was a bit trigger happy with my revert/fix cycle I've restore the original commit.