Page MenuHomePhabricator

Allow rematerialization of ARM Thumb literal pool loads
ClosedPublic

Authored by philipginsbach on Jun 6 2017, 5:16 AM.

Details

Summary

Constants are crucial for code size in the ARM Thumb-1 instruction set.
The 16 bit instruction size often does not offer enough space for immediate arguments.
This means that additional instructions are frequently used to load constants into registers.
Since constants are hoisted, this can lead to significant register spillage if they are used multiple times in a single function.
This can be avoided by rematerialization, i.e. recomputing a constant instead of reloading it from the stack.
This patch fixes the rematerialization of literal pool loads in the ARM Thumb instruction set.

Diff Detail

Repository
rL LLVM

Event Timeline

philipginsbach created this revision.Jun 6 2017, 5:16 AM
samparker edited edge metadata.Jun 21 2017, 6:56 AM

Hi Philip,

Could you re-upload the patch with some added context please? Something like -U100 would be great.

cheers,
sam

Hi Philip,

Are these tests standalone for just this patch, or in conjunction with D33935? If it's difficult to test separately (I'm assuming so), I think you should combine these two patches so they can be committed and tested together.

cheers,
sam

philipginsbach added a comment.EditedJul 13 2017, 4:10 AM

Hi Sam,

The patches are related in their purpose but completely separate in the functionality that they actually touch.
Both patches therefore now have a different test, there is no need to merge them.

Best,
Philip

Ok, great. I've just got some inline queries.

test/CodeGen/Thumb/litpoolremat.ll
24 ↗(On Diff #106389)

Maybe we also check that there aren't any stack spills and restores?

test/CodeGen/Thumb/select.ll
77 ↗(On Diff #106389)

Can you explain why this test has changed please?

philipginsbach added inline comments.Jul 13 2017, 5:44 AM
test/CodeGen/Thumb/litpoolremat.ll
24 ↗(On Diff #106389)

That could be done as well, but I think this more specifically checks the exact functionality of the patch, i.e. to avoid spilling of literal pools loads (and only of them).
For example, if a fourth line of get_value() was added in the middle, the function would still spill after the patch, but at least it would spill once less.

test/CodeGen/Thumb/select.ll
77 ↗(On Diff #106389)

Essentially there are two things going on here:

  1. The select instruction turns into two conditional branches due to the double being split into two 4-byte chunks. This is unfortunate but not in the scope of this patch.
  2. The original code writes -1.0 into the output registers and then conditionally overwrites it with the value of %b. This results in two blt branches (with the same condition %a < 1.234). After the patch, the second chunk is reversed, i.e. it is initialized with part of %b and conditionally overwritten with part of constant -1.0. This is because the second chunk of constant -1.0 is put into a literal pool and loading stuff from there is more expensive than just moving around registers. Before the patch, the compiler isn't aware that it is allowed to make the load from literal pools conditional as it assumes that it is effectful. For the first chunk of -1.0 this is irrelevant, as it is 0x00000000 and thus can be loaded as an immediate.
samparker added inline comments.Jul 13 2017, 6:09 AM
test/CodeGen/Thumb/litpoolremat.ll
24 ↗(On Diff #106389)

Ok, I asked a bad question, I mean why aren't we testing that the immediate is not spilled once its loaded? It would also be good to check what data is at LCPIO_0.

philipginsbach added inline comments.Jul 13 2017, 7:05 AM
test/CodeGen/Thumb/litpoolremat.ll
24 ↗(On Diff #106389)

You are definitely right that it would be nice to verify that LCPIO_0 is indeed where the constant 1764 is stored.
I am not sure how to do that in the FileCheck syntax though, I will look that up.
As for explicitly checking whether it is spilled instead of checking whether it is rematerialized (which is what the test checks right now) I think this does the job and since I emphasized the rematerialization in the patch title it is the preferred way.

philipginsbach added inline comments.Jul 13 2017, 7:21 AM
test/CodeGen/Thumb/litpoolremat.ll
24 ↗(On Diff #106389)

I added the check for LCPIO_0.
As for explicitly checking whether it is spilled instead of checking whether it is rematerialized (which is what the test checks right now) I think this does the job and since I emphasized the rematerialization in the patch title it is the preferred way.

samparker added inline comments.Jul 13 2017, 7:28 AM
test/CodeGen/Thumb/litpoolremat.ll
24 ↗(On Diff #106389)

I didn't mean you should do the spill check instead of what you have done, I just meant as a supplement. It would be nice to have the extra explicit check that there's no 'str r0, [sp]' after the initial load.

for the literal pool check, you could perform the check after the code checks with something like:
CHECK-LABEL: LCPI0_0
CHECK-NEXT: . long 1764

philipginsbach added inline comments.
test/CodeGen/Thumb/litpoolremat.ll
24 ↗(On Diff #106389)

Agreed, I added both.

samparker accepted this revision.Jul 13 2017, 7:45 AM

Great, thanks, LGTM!

This revision is now accepted and ready to land.Jul 13 2017, 7:45 AM

Thanks a lot Sam!

This revision was automatically updated to reflect the committed changes.