This is an archive of the discontinued LLVM Phabricator instance.

[ELF] - Implemented support for test/binop relaxations from latest ABI.
ClosedPublic

Authored by grimar on May 30 2016, 9:42 AM.

Details

Summary

Patch implements next relaxation from latest ABI:

"Convert memory operand of test and binop into immediate operand, where binop is one of adc, add, and, cmp, or,
sbb, sub, xor instructions, when position-independent code is disabled."

It is described in System V Application Binary Interface AMD64 Architecture Processor
Supplement Draft Version 0.99.8 (https://github.com/hjl-tools/x86-psABI/wiki/x86-64-psABI-r249.pdf,
B.2 "B.2 Optimize GOTPCRELX Relocations").

The last relaxation to do after that afaik is:
"When PIC is disabled and foo is defined locally in the lower 32 bit address space, memory operand in mov can be converted into immediate operand."

Diff Detail

Repository
rL LLVM

Event Timeline

grimar updated this revision to Diff 58971.May 30 2016, 9:42 AM
grimar retitled this revision from to [ELF] - Implemented support for test/binop relaxations from latest ABI..
grimar updated this object.
grimar added reviewers: rafael, ruiu.
grimar added subscribers: llvm-commits, grimar.
grimar updated this revision to Diff 59076.May 31 2016, 8:38 AM
  • Do not relax 32bit no-PIC case (as it unknown what can use such instructions, llvm-mc does not generate relaxable relocations types for them).
  • Updated test to use llvm-mc instead of precompiled binary.
  • Minor redesign and comments adjustment.
rafael added inline comments.May 31 2016, 10:20 AM
ELF/InputSection.cpp
260 ↗(On Diff #59076)

This is the same value as R_ABS, move it there.

ELF/Relocations.cpp
247 ↗(On Diff #59076)

This one is not RelExpr if it has the same value as R_ABS.

ELF/Relocations.h
42 ↗(On Diff #59076)

Maybe just name this R_RELAX_GOT.

rafael added inline comments.May 31 2016, 11:13 AM
ELF/Target.cpp
746 ↗(On Diff #59076)

A mov can be relaxed into a "mov $" if we are producing a non pic output. No need to do it now, but add a fixme about it.

797 ↗(On Diff #59076)

These are for the encoding? Maybe refer the official intel docs?

801 ↗(On Diff #59076)

Which instruction is this? testq?

test/ELF/gotpc-relax-nopic.s
92 ↗(On Diff #59076)

Not sure there is any value is testing movl in here. We know we don't produce a relaxable relocation with mc.

grimar updated this revision to Diff 59194.Jun 1 2016, 3:48 AM
grimar updated this object.
grimar marked 6 inline comments as done.
  • Addressed Rafael's review comments.
ELF/InputSection.cpp
260 ↗(On Diff #59076)

Done.

ELF/Relocations.cpp
247 ↗(On Diff #59076)

Removed.

ELF/Target.cpp
746 ↗(On Diff #59076)

Done.

797 ↗(On Diff #59076)

Done.
(Btw we are using (1) in comments about TLS relaxations.)

801 ↗(On Diff #59076)

Yes, it is testq.

test/ELF/gotpc-relax-nopic.s
92 ↗(On Diff #59076)

Did it to show the difference between lld and other linkers here. Do not have strong opinion about real value, so removed.

rafael accepted this revision.Jun 1 2016, 9:32 AM
rafael edited edge metadata.

LGTM with nits

ELF/Target.cpp
759 ↗(On Diff #59194)

You can drop the Type == R_X86_64_REX_GOTPCRELX making this just

return Config->Pic ? RelExpl : R_RELAX_GOT_PC_NOPIC;

no?

782 ↗(On Diff #59194)

git-clang-format

test/ELF/gotpc-relax-nopic.s
16 ↗(On Diff #59194)

Comment out of date.

This revision is now accepted and ready to land.Jun 1 2016, 9:32 AM
This revision was automatically updated to reflect the committed changes.
grimar marked 3 inline comments as done.
grimar added a comment.Jun 1 2016, 9:52 AM

LGTM with nits

Thanks for review !
r271405 and waiting for buildbot results, just in case.

ELF/Target.cpp
759 ↗(On Diff #59194)

Yes. I placed assert there instead to make clear what we expect.
Since we don't have testcase part for 32x instructions anymore, and llvm-mc
generating slightly different result from gas, I think it is reasonable.

grimar updated this object.Jun 1 2016, 9:58 AM
grimar edited edge metadata.