This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Avoid injecting constant islands in movw+movt pairs on Windows
ClosedPublic

Authored by mstorsjo on Aug 21 2018, 3:30 AM.

Details

Summary

On Windows, movw+movt pairs with relocations are handled with a single relocation that covers them both. Therefore we can't inject anything between these instructions, otherwise the relocation (which in LLVM only is treated as the movw instruction's relocation, while the movt instruction's relocation is dropped) will end up bogus. These instructions are bundled up until right before the constant islands pass, making this effectively the only place that can split them apart.

Diff Detail

Event Timeline

mstorsjo created this revision.Aug 21 2018, 3:30 AM

The code changes look good to me, although someone more familiar with the ConstantIslands pass may have a better idea of how to prevent splitting. The only other thought about the test is to make a MIR test and just run the constant-islands pass. For example see the test constant-islands-cfg.mir. I suspect that it will be hard to write the test but it should be a lot less fragile.

test/CodeGen/ARM/constant-island-movwt.ll
303 ↗(On Diff #161680)

If it isn't possible to test this any other way, I suggest adding a comment describing what the test is looking for as it isn't obvious without reference to this review. It may also be possible to loosen some of the conditions such as specific register numbers and presence of other instructions like vdup that might cause a spurious failure if there are small changes.

Can we change the way we model these movw+movt pairs so it isn't so fragile? If these pairs are illegal to split on Windows, I'd prefer to model it as a pseudo-instruction until the asmprinter. Otherwise you're going to be fighting obscure issues in every pass that runs after ARMExpandPseudo.

Can we change the way we model these movw+movt pairs so it isn't so fragile? If these pairs are illegal to split on Windows, I'd prefer to model it as a pseudo-instruction until the asmprinter. Otherwise you're going to be fighting obscure issues in every pass that runs after ARMExpandPseudo.

That's probably the best way forward, although I do feel that's probably a rather large change - I don't know off-hand where to start with that.

The code changes look good to me, although someone more familiar with the ConstantIslands pass may have a better idea of how to prevent splitting. The only other thought about the test is to make a MIR test and just run the constant-islands pass. For example see the test constant-islands-cfg.mir. I suspect that it will be hard to write the test but it should be a lot less fragile.

Actually I think I can generate that test from the one I have now, with llc -stop-before=arm-cp-islands. I'll try to get the test updated with that. Since nobody seems to have run into this before, I'm pondering if this fix could be acceptable until a proper restructuring with a pseudoinstruction can be done.

That's probably the best way forward, although I do feel that's probably a rather large change - I don't know off-hand where to start with that.

Probably just disable the code to expand it in ARMExpandPseudo if the target is Windows, add code to emit the pseudo-instruction in ARMAsmPrinter, and fix ARMBaseInstrInfo::getInstSizeInBytes to return the right result. I don't think you'd actually need to change anything else; nothing should modify an unknown pseudo-instruction.

Since nobody seems to have run into this before, I'm pondering if this fix could be acceptable until a proper restructuring with a pseudoinstruction can be done.

Looking a bit more closely, it looks like we bundle the instructions on Windows, so actually the window for other passes to mess with the unbundled pair is pretty small (just passes between unbundling and the asmprinter, which is basically just ConstantIslands). So maybe this is good enough without a new pseudo-instruction.

lib/Target/ARM/ARMConstantIslandPass.cpp
1427

Probably should mention that the instructions are bundled until just before ConstantIslands.

1429

The "MI->getNumOperands() >= 3" is probably unnecessary.

1432

Assert that MI is a MOVW?

Since nobody seems to have run into this before, I'm pondering if this fix could be acceptable until a proper restructuring with a pseudoinstruction can be done.

Looking a bit more closely, it looks like we bundle the instructions on Windows, so actually the window for other passes to mess with the unbundled pair is pretty small (just passes between unbundling and the asmprinter, which is basically just ConstantIslands). So maybe this is good enough without a new pseudo-instruction.

Ah, thanks - that explains why it hasn't exploded completely before. I've only run into this with compiling LLVM itself for this target, where certain source files seemed to trigger it.

lib/Target/ARM/ARMConstantIslandPass.cpp
1427

Ok, will do

1429

Will remove

1432

Sure

mstorsjo updated this revision to Diff 161892.Aug 22 2018, 12:54 AM
mstorsjo edited the summary of this revision. (Show Details)

Applied @efriedma's suggestions, changed the testcase to a MIR test which only runs the arm-cp-islands pass on it, making the test much less fragile.

To make MIR work properly for thumb(2), I had to move metadata about the target flags from ARMInstrInfo to ARMBaseInstrInfo. I can commit that part in a separate commit if you prefer that.

Thanks for the test update, I've no further comments.

test/CodeGen/ARM/constant-island-movwt.mir
885

Could there be a comment to say something like. Check that constant pool is not placed in between t2MOVi16 and t2MOVTi16 as Windows requires these to be consecutive?

mstorsjo added inline comments.Aug 22 2018, 2:22 AM
test/CodeGen/ARM/constant-island-movwt.mir
885

Sure, I'll amend it locally with such a comment.

This revision is now accepted and ready to land.Aug 22 2018, 11:58 AM
This revision was automatically updated to reflect the committed changes.