[PATCH] Adapt MipsLongBranch pass to work in NaCl.

[Mark Seaborn]

================
Comment at: lib/Target/Mips/MipsLongBranch.cpp:70
@@ -69,2 +69,3 @@
         ABI(TM.getSubtarget<MipsSubtarget>().getTargetABI()),
-        LongBranchSeqSize(!IsPIC ? 2 : (ABI == MipsSubtarget::N64 ? 13 : 9)) {}
+        LongBranchSeqSize(!IsPIC ? 2 : (ABI == MipsSubtarget::N64 ? 13 :
+            (!TM.getSubtarget<MipsSubtarget>().isTargetNaCl() ? 9 : 10))) {}
----------------
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...

================
Comment at: test/MC/Mips/nacl-long-branch.ll:1
@@ +1,2 @@
+; RUN: llc -filetype=obj -O3 -mtriple mipsel-none-linux < %s \
+; RUN:   | llvm-objdump -triple mipsel -disassemble -no-show-raw-insn - \
----------------
Doesn't this test belong in test/CodeGen, since it's testing more than the MC layer?

================
Comment at: lib/Target/Mips/MipsLongBranch.cpp:497
@@ +496,3 @@
+        // that will be added later in the MC layer.  Since at this point we
+        // don't know the exact amount of code that "sanboxing" will add, we
+        // conservatively estimate that code will not grow more than 100%.
----------------
"sandboxing"

================
Comment at: lib/Target/Mips/MipsMCInstLower.cpp:187
@@ +186,3 @@
+  // Lower two register operands.
+  for (unsigned i = 0, e = 2; i != e; ++i) {
+    const MachineOperand &MO = MI->getOperand(i);
----------------
Names should be "I" and "E" to follow LLVM style

================
Comment at: test/MC/Mips/nacl-long-branch.ll:2
@@ +1,3 @@
+; RUN: llc -filetype=obj -O3 -mtriple mipsel-none-linux < %s \
+; RUN:   | llvm-objdump -triple mipsel -disassemble -no-show-raw-insn - \
+; RUN:   | FileCheck %s
----------------
Why not test llc's assembly output rather than its object file output?  That should be easier to read and maintain.

================
Comment at: lib/Target/Mips/MipsLongBranch.cpp:303
@@ +302,3 @@
+          .append(BuildMI(*MF, DL, TII->get(Mips::BAL_BR)).addMBB(BalTgtMBB))
+          .append(BuildMI(*MF, DL, TII->get(Mips::LUi), Mips::AT).addImm(Hi));
+
----------------
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?


http://llvm-reviews.chandlerc.com/D3089