This is an archive of the discontinued LLVM Phabricator instance.

[MC] Relax .fill size requirements
ClosedPublic

Authored by niravd on May 10 2018, 9:37 AM.

Details

Summary

Avoid requirement that number of values to fill must be known as assembler time.

Fixes PR33586.

Diff Detail

Event Timeline

niravd created this revision.May 10 2018, 9:37 AM
rnk added inline comments.May 10 2018, 10:55 AM
llvm/lib/MC/MCObjectStreamer.cpp
646–662

Would it be cleaner to generalize MCFillFragment and then we could defer this label difference calculation until after relaxation?

llvm/lib/MC/MCParser/AsmParser.cpp
3144 ↗(On Diff #146146)

Given that this is the only change in the file, I'd revert it.

peter.smith added inline comments.May 11 2018, 2:57 AM
llvm/lib/MC/MCObjectStreamer.cpp
646–662

I tend to agree with rnk here. I'm guessing that if there are size concerns about adding fields to MCFillFragment that are not commonly used then it could be worth a new fragment type, potentially derived from MCFillFragment.

647

Given that NumValues is passed to emitFill(); did you mean // Byte fills don't need to know IntNumValues?

llvm/test/MC/X86/pr33586.s
1 ↗(On Diff #146146)

The test looks very similar to the attachment in the PR. Given that the code was from the Linux Kernel we probably ought to generalise and simplify it to avoid any GPL concerns.

niravd updated this revision to Diff 146648.May 14 2018, 11:42 AM
niravd marked an inline comment as done.
niravd edited the summary of this revision. (Show Details)

Generalize to non-byte fills as well.

void added a subscriber: void.May 14 2018, 12:03 PM

Thanks for the update. I've left some minor comments but I'm overall happy with the change.

llvm/lib/MC/MCAssembler.cpp
563–587

Maybe worth a comment summarising the specification of how .fill forms the value, or hint that the interested reader should look at the GNU as directives manual for a description.

568

I suggest removing the newline here as both loops are constructing Data.

572

Perhaps // Set ChunkSize to largest multiple of VSize in Data
or do unsigned ChunkSize = MaxChunkSize - MaxChunkSize % VSize
It could also be const unsigned.

578

I don't think you need the {} in this case.

581

Could be possible to compress without losing readability, although feel free to ignore as this is just a style choice:
if (unsigned TrailingCount = FragmentSize % ChunkSize)

OW->writeBytes(StringRef(Data, TrailingCount));
llvm/lib/MC/MCObjectStreamer.cpp
666–668

This bit of code is almost identical to emitFill above, the only difference being that Size is used rather than 1. It may be worth passing Size to emitFill above (perhaps with a default value of 1).

llvm/test/MC/AsmParser/assembler-expressions.s
57

I think we haven't got a big-endian test for .fill when the size is > 1. I think there is a Mips test that uses .fill (hilo-addressing) but it is only .fill 0x30124-8

Probably worth adding one to make sure that the endian reversal code is working.

niravd updated this revision to Diff 147169.May 16 2018, 1:34 PM
niravd marked 5 inline comments as done.

Address comments. The only pending comment is regards a big-endian example.

niravd added inline comments.May 16 2018, 1:45 PM
llvm/test/MC/AsmParser/assembler-expressions.s
57

For this test to be properly exercised there must be a assembler-time unknown expression that we can resolve at layout time. AFAIK this must be a text section ambiguity due to instruction relaxation. The only in-tree big-endian machine that does relax instructions is ARM. It's proving to not be a simple transliteration of the other test though.

Thanks for the update, typo aside they look fine to me.

If it helps here is a program that should show relaxation on a big endian arm system:

// RUN: llvm-mc --triple=thumbv7eb-linux-gnueabihf %s -filetype=obj -o %t
// RUN: llvm-objdump -triple=thumbv7eb-linux-gnueabihf -d %t
        .syntax unified
        .text
        .thumb
.L1:
        beq Label
.L2:
        .word 0
        .fill (.L2 - .L1)/4, 4, 0x12345678
        .space 1024
Label:

llvm-objdump doesn't seem to be able to disassemble the instructions properly, but by using .word to force a .data mapping symbol it will disassemble the .fill. Big endian on an Armv7 is strange in that the object files have big-endian instructions but are reversed at link time to little-endian so I'm not surprised it doesn't handle this well.

llvm/lib/MC/MCAssembler.cpp
563

Typo: verctor -> vector

niravd updated this revision to Diff 147332.May 17 2018, 9:21 AM

Fix typo. Add big-endian ARM test that avoids llvm-objdump big-endian thumb issues.

peter.smith accepted this revision.May 18 2018, 1:28 AM

Thanks for the update, looks good to me.

This revision is now accepted and ready to land.May 18 2018, 1:28 AM
This revision was automatically updated to reflect the committed changes.