[mips] Fix MipsLongBranch pass to work when the offset from the branch to the target cannot be determined accurately. This is the case for NaCl where the sandboxing instructions are added in MC layer, after the MipsLongBranch pass. It is also the case when the code has inline assembly. Instead of calculating offset in the MipsLongBranch pass, use %hi(sym1 - sym2) and %lo(sym1 - sym2) expressions that are resolved during the fixup.
Details
Diff Detail
Event Timeline
lib/Target/Mips/MipsLongBranch.cpp | ||
---|---|---|
67 | For readability, this is a bit too complex to be an expression now. How about turning it into a statement? Also the magic numbers could do with some explaining. However, maybe some of the magic numbers can go away? See other comment... | |
325 | So, the existing code has some hairy logic for calculating the offset to use here. It's based on counting the number of instructions in MachineBasicBlocks. However, that doesn't work when targeting NaCl, because bundle padding changes the offsets. I'd also guess that inline assembly breaks the hairy calculations. There's a comment at the top: You've implemented a version which works without the hairy offset calculations. This seems like a good opportunity to remove them, and only use your new label-based calculations. How does that sound? | |
460 | "sandboxing" | |
lib/Target/Mips/MipsMCInstLower.cpp | ||
186 | Names should be "I" and "E" to follow LLVM style | |
test/MC/Mips/nacl-long-branch.ll | ||
1 ↗ | (On Diff #7857) | Doesn't this test belong in test/CodeGen, since it's testing more than the MC layer? |
2 ↗ | (On Diff #7857) | Why not test llc's assembly output rather than its object file output? That should be easier to read and maintain. |
test/MC/Mips/nacl-long-branch.ll | ||
---|---|---|
2 ↗ | (On Diff #7857) | I agree we should be using llc's assembly output. In general, 'llc -filetype=obj' tests should be split into separate CodeGen and MC parts. We need to be able to assemble anything CodeGen emits. |
I removed NaCl-specific parts from the patch. NaCl code will be added in a separate patch.
Can you add more background to the commit message? The primary motivation was to support NaCl sandboxing. Handling inline asm is really just a side benefit, as is simplification of the code.
lib/Target/Mips/AsmParser/MipsAsmParser.cpp | ||
---|---|---|
1394 ↗ | (On Diff #8098) | So the MC-layer feature you added earlier was insufficient and so you're adding more supported cases here, presumably for 64-bit MIPS? This is the kind of thing that you should explain in the commit message. Can you split the MC-layer part for handling %higher/%highest out into a separate change? |
1397 ↗ | (On Diff #8098) | It looks like this could be done more cleanly with a switch() statement. |
lib/Target/Mips/Mips64InstrInfo.td | ||
240 | Could you add comments to say what these two pseudo-ops do? | |
245 | Nit: having two empty lines here is inconsistent with the surrounding code | |
lib/Target/Mips/MipsAsmPrinter.cpp | ||
153 | isLongBranchPseudo() doesn't need to be a method, does it? If it were static you wouldn't need the casting above. | |
lib/Target/Mips/MipsInstrInfo.td | ||
881 | Ditto: add comments here? | |
lib/Target/Mips/MipsLongBranch.cpp | ||
12–13 | Doesn't need a number if you remove point 2. | |
176 | This seems like the wrong place to put a FIXME, because if you do the fix, you won't be changing this part of the code, will you? So anyone implementing fixing this could miss removing the FIXME. If the problem is a missing method override, I'd suggest just leaving out the FIXME. You could file an issue for what you're working on and list the tasks involved there. | |
266–267 | I think this isn't needed any more. See other comment about the assert(). | |
382 | Should this become the following? assert(LongBrMBB->size() + BalTgtMBB->size() == LongBranchSeqSize); | |
lib/Target/Mips/MipsMCInstLower.cpp | ||
200 | Would this be better as a switch()? | |
210 | Nit: align indentation with "(" | |
231 | If you're doing style cleanups to existing code, it would be better to commit that as a separate change. | |
test/CodeGen/Mips/longbranch.ll | ||
7 | Can you explain what's changing in the test case? The changes to the test input look like they might be unnecessary, but I can't tell. Could you make the test changes more minimal? |
test/CodeGen/Mips/longbranch.ll | ||
---|---|---|
7 | I simplified the fallthrough block (if.then) so that it has 2 bitcode instructions instead of 4, and has 3 assembly instructions instead of 4 (one less CHECK). I also renamed the global variable g0 to x because in the assembly output there is a relocation %got(g0) and I thought that %got(x) would be more readable. I also renamed the test from foo1 to test1. |
The code that extends MipsMCExpr class to handle %higher and %highest relocations is split into a separate patch.
The code that extends MipsMCExpr class to handle %higher and %highest relocations is split into a separate patch.
Thanks for splitting that out (in http://llvm-reviews.chandlerc.com/D3230). It occurs to me now that you only need %higher/%highest if the jump offset is >4GB, which seems rather unlikely, if not downright impossible when compiling a single function. You'd want to handle functions bigger than 64k but not functions bigger than 4GB. :-)
Given that, would it make sense to drop the %higher/%highest instructions from the code sequence you generate on MIPS64, making the code a little smaller and faster?
Would it make sense to drop the %higher/%highest instructions from the code sequence you generate on
MIPS64, making the code a little smaller and faster?
I added TODO comment for this; I'll implement it in a separate patch.
I reordered instructions in the long branch to avoid the bug in GAS. When first of the symbols in %hi(label1 - label2) and %lo(label1 - label2) is defined before the two instructions, and the other symbol is defined between the two instructions, then GAS can sometime (for label1 - label2 close to -32768) wrongly calculate %hi(label1 - label2), by adding +1 when it is not needed. To avoid this, instead of
$longbr:
addiu $sp, $sp, -8 sw $ra, 0($sp) bal $baltgt lui $at, %hi($tgt - $baltgt)
$baltgt:
addiu $at, $at, %lo($tgt - $baltgt) addu $at, $ra, $at lw $ra, 0($sp) jr $at addiu $sp, $sp, 8
$fallthrough:
the following sequence is used for long branch:
$longbr:
addiu $sp, $sp, -8 sw $ra, 0($sp) lui $at, %hi($tgt - $baltgt) bal $baltgt addiu $at, $at, %lo($tgt - $baltgt)
$baltgt:
addu $at, $ra, $at lw $ra, 0($sp) jr $at addiu $sp, $sp, 8
$fallthrough:
LGTM
lib/Target/Mips/Mips64InstrInfo.td | ||
---|---|---|
243 | Can you add a comment like this: | |
246 | This is the least obvious pseudo-op, so please add a comment like this: Expands to: addiu $dst, $src, %PART($tgt - $baltgt) | |
lib/Target/Mips/MipsInstrInfo.td | ||
930 | Can you add a comment like: | |
933 | Similarly: | |
lib/Target/Mips/MipsMCInstLower.cpp | ||
171 | When would this be false? Should this be an assertion instead, or should this check be omitted? | |
189 | Same here | |
218 | It would be slightly more robust to do: else if (TargetFlags == MipsII::MO_ABS_LO) ... else report_fatal_error("Unexpected flags for LONG_BRANCH_DADDiu"); | |
test/CodeGen/Mips/longbranch.ll | ||
3 | Nit: can you indent continuation lines by 2 or 4 spaces, rather than 1, for readability? | |
53 | Maybe add a comment before this line:
| |
54 | Can you make these O32-NEXT for as many of the long-branch-expansion lines as possible, to make the test stricter? (Same for the other test cases too.) |
Patch is updated. I applied all the comments. I also changed the width of some lines to fit in 80 columns.
lib/Target/Mips/Mips64InstrInfo.td | ||
---|---|---|
243 | Done | |
246 | Done | |
lib/Target/Mips/MipsInstrInfo.td | ||
930 | Done | |
933 | Done | |
lib/Target/Mips/MipsMCInstLower.cpp | ||
171 | I removed the check. It can be false when the instruction has implicit operands, but none of the LONG_BRANCH pseudos has implicit operands. | |
189 | I removed the check. | |
218 | Done | |
test/CodeGen/Mips/longbranch.ll | ||
3 | Done | |
53 | Done | |
54 | Done |
Could you add comments to say what these two pseudo-ops do?