Page MenuHomePhabricator

[AArch64] Return the correct size for TLSDESC_CALLSEQ
ClosedPublic

Authored by rovka on Jul 27 2016, 8:40 AM.

Details

Summary

The branch relaxation pass is computing the wrong offsets because it assumes
TLSDESC_CALLSEQ eats up 4 bytes, when in fact it is lowered to an instruction
sequence taking up 16 bytes. This can become a problem in huge files with lots
of TLS accesses, as it may slowly move branch targets out of the range computed
by the branch relaxation pass.

Fixes PR24234.

Diff Detail

Repository
rL LLVM

Event Timeline

rovka updated this revision to Diff 65749.Jul 27 2016, 8:40 AM
rovka retitled this revision from to [AArch64] Return the correct size for TLSDESC_CALLSEQ.
rovka updated this object.
rovka added subscribers: llvm-commits, rengolin, emaste.
rovka added a comment.Jul 27 2016, 8:47 AM

It would be nice if we could identify other situations where this might happen - I'm not convinced GetInstSizeInBytes is catching all the special cases. At a quick glance through AArch64AsmPrinter, I think STACKMAP and PATCHPOINT might be other offenders. Is it worth looking into those as well?

On another line of thought, the original file [1] causing this bug is pretty huge and it might catch other things like this in the future - do we want to add it or something similar to the test-suite?

[1] https://llvm.org/bugs/attachment.cgi?id=15581

lib/Target/AArch64/AArch64.h
51 ↗(On Diff #65749)

This won't be necessary if D22868 goes through, but I added it here to keep the patch self-contained (without this we can't run the test).

t.p.northover accepted this revision.Jul 27 2016, 9:15 AM
t.p.northover edited edge metadata.

It would be nice if we could identify other situations where this might happen - I'm not convinced GetInstSizeInBytes is catching all the special cases.

I'm convinced it doesn't. Its heuristic for inline asm is flat-out wrong from what I remember (count the number of lines and multiply by 4).

Anyway, the patch looks fine with just one minor comment change in case anyone has to read it again:

test/CodeGen/MIR/AArch64/inst-size-tlsdesc-callseq.mir
27–28 ↗(On Diff #65749)

Could you reiterate that you're using aarch64-tbz-offset-bits here. I was thoroughly confused, and spent 5 minutes looking for the massive space-consuming instruction below.

This revision is now accepted and ready to land.Jul 27 2016, 9:15 AM
rovka added inline comments.Jul 28 2016, 12:05 AM
test/CodeGen/MIR/AArch64/inst-size-tlsdesc-callseq.mir
27–28 ↗(On Diff #65749)

Sorry about that! I'll fix it before I commit, thanks for reviewing.

This revision was automatically updated to reflect the committed changes.