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.
Details
Details
Diff Detail
Diff Detail
- Repository
- rL LLVM
Event Timeline
Comment Actions
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. |
Comment Actions
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.
Comment Actions
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.
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.
Comment Actions
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 ↗ | (On Diff #161680) | Probably should mention that the instructions are bundled until just before ConstantIslands. |
| 1429 ↗ | (On Diff #161680) | The "MI->getNumOperands() >= 3" is probably unnecessary. |
| 1432 ↗ | (On Diff #161680) | Assert that MI is a MOVW? |
Comment Actions
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 ↗ | (On Diff #161680) | Ok, will do |
| 1429 ↗ | (On Diff #161680) | Will remove |
| 1432 ↗ | (On Diff #161680) | Sure |
Comment Actions
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.
Comment Actions
Thanks for the test update, I've no further comments.
| test/CodeGen/ARM/constant-island-movwt.mir | ||
|---|---|---|
| 885 ↗ | (On Diff #161892) | 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? |
| test/CodeGen/ARM/constant-island-movwt.mir | ||
|---|---|---|
| 885 ↗ | (On Diff #161892) | Sure, I'll amend it locally with such a comment. |