This is an archive of the discontinued LLVM Phabricator instance.

Fix -fPIC handling on arm64
Needs ReviewPublic

Authored by phcoder on Aug 10 2017, 6:01 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

phcoder created this revision.Aug 10 2017, 6:01 AM
rovka added a subscriber: rovka.Aug 10 2017, 6:14 AM

Hi,

Thanks for sending the patch.

When submitting a patch, you should upload with context, so people can look at the rest of the code easily, and add llvm-commits as a subscriber, so more people get to see your patch. This is described here:
https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

Please also update your patch with tests and a descriptive summary explaining what the current behaviour is, why it is wrong and how your patch fixes it. The link to the bug report is good, but it doesn't say enough.

A brief overview of what regression tests should look like is here:
https://llvm.org/docs/TestingGuide.html#writing-new-regression-tests

You might also want to look at existing tests for examples (test/CodeGen/AArch64 would be the place to look in this case).

Hope that helps,
Diana

t.p.northover added a subscriber: t.p.northover.

Also, make sure "llvm-commits" is a subscriber.

peter.smith edited edge metadata.Aug 10 2017, 7:26 AM

As well as strings it seems like absolute references are also used for global variables that are DSO local. Always going via the GOT is one possible solution, however I don't think it is necessarily the best solution as there are PC relative versions of the MOVW relocations such as R_AARCH64_MOVW_PREL_G0. These could be used to load the offset to the DSO local rather than going via the GOT. Unfortunately these relocations don't seem to be implemented yet in AArch64 so it looks like going via the GOT is better than producing a non-PIC sequence.

However while looking at the relative cost of using a PC-relative sequence of MOV/MOVK versus going via the .got I noticed that even for -mcmodel=large we are using the sequence:

	adrp	x8, :got:glob
	ldr	x8, [x8, :got_lo12:glob]
	ldr		w0, [x8]

If your definition of -mcmodel=large is "no limit on segment size" then this sequence doesn't qualify as the adrp has a limit of 4Gb. As I understand it we would have to use PC-relative MOV, MOVK or a load from a literal pool to access the GOT rather than use ADRP.

If we permit the use of ADRP in -mcmodel=large to access the GOT then when using PIC why not use the small code sequence way to access DSO locals with:

	adrp	x0, .L.str
	add	x0, x0, :lo12:.L.str
	ret
lib/Target/AArch64/AArch64Subtarget.cpp
128

To me the comment implies that we are masking the problem that an absolute code-sequence is chosen for PIC by choosing an alternative code sequence, I think we need a bit more detail about why we are making the choice.