This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Emmit data symbol for constant pool data
ClosedPublic

Authored by dnsampaio on Oct 1 2018, 11:26 AM.

Details

Summary

The ARM elf emitter would omit printing data symbol when constant data. This patch overrides the emitFill method as to enforce that the symbol is correctly printed.

Diff Detail

Repository
rL LLVM

Event Timeline

dnsampaio created this revision.Oct 1 2018, 11:26 AM
olista01 added inline comments.Oct 2 2018, 1:44 AM
lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp
1481 ↗(On Diff #167785)

Why can't we just call EmitDataMappingSymbol, like EmitBytes does?

test/CodeGen/ARM/CheckDataSymbol.ll
1 ↗(On Diff #167785)

I don't think this test is necessary, since this can be tested with an assembly file.

test/CodeGen/ARM/CheckDataSymbol.s
17 ↗(On Diff #167785)

This test can be reduced greatly, to something like this:

.text
nop
.zero 4
dnsampaio marked 3 inline comments as done.Oct 2 2018, 2:52 AM
dnsampaio added inline comments.
lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp
1481 ↗(On Diff #167785)

Indeed it can.

dnsampaio updated this revision to Diff 167905.Oct 2 2018, 2:54 AM
dnsampaio marked an inline comment as done.

Simplified tests. Using the standard function for emitting data symbols, if required.

olista01 added inline comments.Oct 2 2018, 3:11 AM
test/CodeGen/ARM/CheckDataSymbol.s
1 ↗(On Diff #167905)

We can't use clang in LLVM tests (LLVM can be built without clang), this should use llvm-mc instead. Also, this should be in tests/MC/ARM, not tests/CodeGen/ARM, since it's testing stuff in the MC layer.

dnsampaio updated this revision to Diff 167912.Oct 2 2018, 3:38 AM
dnsampaio marked an inline comment as done.

Updated test to use llvm-mc instead of clang.

olista01 accepted this revision.Oct 2 2018, 7:46 AM

LGTM with one nit.

test/MC/ARM/CheckDataSymbol.s
1 ↗(On Diff #167912)

The -arm-implicit-it=thumb option isn't needed here.

This revision is now accepted and ready to land.Oct 2 2018, 7:46 AM
dnsampaio marked an inline comment as done.Oct 2 2018, 7:56 AM
This revision was automatically updated to reflect the committed changes.