This is an archive of the discontinued LLVM Phabricator instance.

Fix MipsLongBranch pass to work when the code has inline assembly.
ClosedPublic

Authored by sstankovic on Mar 14 2014, 3:18 PM.

Details

Summary

[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.

Diff Detail

Event Timeline

mseaborn added inline comments.Mar 14 2014, 5:07 PM
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:
FIXME: ...
2. If program has inline assembly statements whose size cannot be
// determined accurately, load branch target addresses from the GOT.

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?

464

"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.

dsanders added inline comments.Mar 18 2014, 7:12 AM
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.

sstankovic updated this revision to Unknown Object (????).Mar 20 2014, 10:02 AM

I removed NaCl-specific parts from the patch. NaCl code will be added in a separate patch.

sstankovic updated this revision to Unknown Object (????).Mar 20 2014, 10:18 AM

Changed variable names to uppercase.

sstankovic updated this revision to Unknown Object (????).Mar 25 2014, 10:22 AM

Updated the patch to apply to the current trunk.

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
154

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
885

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().

386

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 "("

233

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?

sstankovic added inline comments.Mar 25 2014, 3:45 PM
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.

sstankovic updated this revision to Unknown Object (????).Mar 31 2014, 12:32 PM

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?

sstankovic updated this revision to Unknown Object (????).Apr 2 2014, 8:04 AM

Patch is updated.

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.

sstankovic updated this revision to Unknown Object (????).Apr 2 2014, 9:45 AM

Fixed the comment.

sstankovic updated this revision to Unknown Object (????).Apr 16 2014, 9:36 AM

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:
// Expands to: lui64 $dst, %highest($tgt - $baltgt)

246

This is the least obvious pseudo-op, so please add a comment like this:

Expands to: addiu $dst, $src, %PART($tgt - $baltgt)
where %PART may be %higher, %hi or %lo, depending on the relocation kind
// that $tgt is annotated with.

lib/Target/Mips/MipsInstrInfo.td
934

Can you add a comment like:
// Expands to: lui $dst, %hi($tgt - $baltgt)

937

Similarly:
// Expands to: addiu $dst, $src, %lo($tgt - $baltgt)

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:
; Check for long branch expansion:

  • just to make it clearer which part is the long branch. Same for the other 2 cases below.
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.)

sstankovic updated this revision to Diff 8895.Apr 28 2014, 1:24 PM
sstankovic edited edge metadata.

Patch is updated. I applied all the comments. I also changed the width of some lines to fit in 80 columns.

sstankovic added inline comments.Apr 28 2014, 1:34 PM
lib/Target/Mips/Mips64InstrInfo.td
243

Done

246

Done

lib/Target/Mips/MipsInstrInfo.td
934

Done

937

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

sstankovic accepted this revision.Apr 30 2014, 9:05 AM
sstankovic added a reviewer: sstankovic.
This revision is now accepted and ready to land.Apr 30 2014, 9:05 AM
sstankovic closed this revision.Apr 30 2014, 9:05 AM
test/CodeGen/Mips/longbranch.ll