Page MenuHomePhabricator

Fix 24560: assembler does not share constant pool for same constants
ClosedPublic

Authored by weimingz on Oct 19 2016, 4:53 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

weimingz updated this revision to Diff 75255.Oct 19 2016, 4:53 PM
weimingz retitled this revision from to Fix 24560: assembler does not share constant pool for same constants.
weimingz updated this object.
weimingz set the repository for this revision to rL LLVM.
weimingz added reviewers: rengolin, eli.friedman.
weimingz added a subscriber: llvm-commits.
jmolloy requested changes to this revision.Oct 20 2016, 3:05 AM
jmolloy added a reviewer: jmolloy.
jmolloy added a subscriber: jmolloy.

Hi,

Please post with full context.

Please add tests that ensure the behaviour is correct when there are multiple references to a constant and one of them is out of range (the constant should be duplicated in this case).

James

This revision now requires changes to proceed.Oct 20 2016, 3:05 AM
weimingz updated this revision to Diff 75318.Oct 20 2016, 10:58 AM
weimingz edited edge metadata.

Forgot to include the full context in previous diff.

Regarding the out-of-range test, give a test like:
foo1:
PUSH {r4-r6,lr}
LDR r7, =0x80808080
.space 0x1000
LDR r7, =0x80808080

Existing llvm-mc gives "error: out of range pc-relative fixup value". Similarly, gnu-as gives "Error: invalid literal constant: pool needs to be closer"
Is our case, if the first LDR is valid (within the range), then following LDR to the same constant should be valid as well since we always put CP at the end.

rengolin edited edge metadata.Oct 24 2016, 3:14 AM

Regarding the out-of-range test, give a test like:
foo1:
PUSH {r4-r6,lr}
LDR r7, =0x80808080
.space 0x1000
LDR r7, =0x80808080

Existing llvm-mc gives "error: out of range pc-relative fixup value". Similarly, gnu-as gives "Error: invalid literal constant: pool needs to be closer"
Is our case, if the first LDR is valid (within the range), then following LDR to the same constant should be valid as well since we always put CP at the end.

I think this makes sense.

The side effects of the caching are of *removing* constants, not adding them, so if anything, the range of the original load should be shorter, not longer. The number of instructions are also the same, so overall, this should never increase the code/data in any way.

Also, whatever relocation didn't work before (negative, wrong range, wrong relocation), will keep not working, but that's nothing to do with the patch itself.

cheers,
--renato

@jmolloy HI James, do you have any comments?

majnemer added inline comments.
lib/MC/ConstantPools.cpp
40–41 ↗(On Diff #75318)

You could save a lookup by using find/end instead of count/operator[]

weimingz updated this revision to Diff 76481.Oct 31 2016, 2:21 PM
weimingz edited edge metadata.

Use find/end as David suggested

No more comments from me

jmolloy accepted this revision.Nov 3 2016, 2:07 AM
jmolloy edited edge metadata.

Explicitly accepting.

This revision is now accepted and ready to land.Nov 3 2016, 2:07 AM
weimingz updated this revision to Diff 76901.EditedNov 3 2016, 4:45 PM
weimingz edited edge metadata.

rebase

This revision was automatically updated to reflect the committed changes.

This commit causes https://bugs.llvm.org//show_bug.cgi?id=32825

Regarding the out-of-range test, give a test like:
foo1:
PUSH {r4-r6,lr}
LDR r7, =0x80808080
.space 0x1000
LDR r7, =0x80808080

Existing llvm-mc gives "error: out of range pc-relative fixup value". Similarly, gnu-as gives "Error: invalid literal constant: pool needs to be closer"
Is our case, if the first LDR is valid (within the range), then following LDR to the same constant should be valid as well since we always put CP at the end.

The above example is invalid, you must use .ltorg to ensure literal pool accessibility.
The corrected(valid) code can be compiled by gnu-as, but not by llvm-mc

foo1:
    PUSH {r4-r6,lr}
    LDR r7, =0x80808080
    .ltorg
    .space 0x1000
    LDR r7, =0x80808080
    .ltorg

Can you, please, fix or revert it? It's a clear regression as the example can be compiled using llvm 3.9 but not by llvm 4.0+..
Thanks.

strejda added a subscriber: dim.May 20 2017, 7:22 AM
jmolloy reopened this revision.May 22 2017, 1:43 AM

PR32825 seems valid and has been open for some time. Weiming, I have reverted this in r303535.

Cheers,

James

This revision is now accepted and ready to land.May 22 2017, 1:43 AM

PR32825 seems valid and has been open for some time. Weiming, I have reverted this in r303535.

Wasn't PR32825 already fixed by rL302416 (D32847)?

jmolloy closed this revision.May 22 2017, 2:44 AM

Re-writing here because phab didn't pick up my email reply.

I have double checked the PR, and yes, it is fixed on ToT. From my perspective what I saw is a commit that I'd LGTM'd with an open PR and no response from the author, which is why I backed it out.

I've recommitted the two backed out diffs and updated PR32825 as resolved.

James

Can you, please, fix or revert it? It's a clear regression as the example can be compiled using llvm 3.9 but not by llvm 4.0+..

WRT to this being a regression in llvm 4.0 - it might be a little late to try to get the fix merged into 4.0.1. Although, the window for opening such a merge request closes today, so if you feel it's urgent, perhaps such a merge request should be opened?

Can you, please, fix or revert it? It's a clear regression as the example can be compiled using llvm 3.9 but not by llvm 4.0+..

WRT to this being a regression in llvm 4.0 - it might be a little late to try to get the fix merged into 4.0.1. Although, the window for opening such a merge request closes today, so if you feel it's urgent, perhaps such a merge request should be opened?

I opened a merge request for this fix in https://bugs.llvm.org/show_bug.cgi?id=33122 in any case.