[PATCH] Fix MipsLongBranch pass to work when the code has inline assembly.

[Mark Seaborn]

  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.


================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:1394
@@ +1393,3 @@
+        && isa<MCSymbolRefExpr>(BE->getRHS())) {
+      if (VK == MCSymbolRefExpr::VK_Mips_HIGHEST)
+        return MipsMCExpr::Create(MipsMCExpr::VK_Mips_HIGHEST, Expr,
----------------
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?

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:1397
@@ +1396,3 @@
+                                  getContext());
+      else if (VK == MCSymbolRefExpr::VK_Mips_HIGHER)
+        return MipsMCExpr::Create(MipsMCExpr::VK_Mips_HIGHER, Expr,
----------------
It looks like this could be done more cleanly with a switch() statement.

================
Comment at: lib/Target/Mips/Mips64InstrInfo.td:237
@@ +236,3 @@
+  (ins GPR64Opnd:$src, brtarget:$tgt, brtarget:$baltgt), []>;
+
+
----------------
Nit: having two empty lines here is inconsistent with the surrounding code

================
Comment at: lib/Target/Mips/Mips64InstrInfo.td:232
@@ -231,1 +231,3 @@
 
+def LONG_BRANCH_LUi64 : PseudoSE<(outs GPR64Opnd:$dst),
+  (ins brtarget:$tgt, brtarget:$baltgt), []>;
----------------
Could you add comments to say what these two pseudo-ops do?

================
Comment at: lib/Target/Mips/MipsAsmPrinter.cpp:153
@@ -151,1 +152,3 @@
+    if (I->isPseudo() && !Subtarget->inMips16Mode()
+        && !TII->isLongBranchPseudo(I->getOpcode()))
       llvm_unreachable("Pseudo opcode found in EmitInstruction()");
----------------
isLongBranchPseudo() doesn't need to be a method, does it?  If it were static you wouldn't need the casting above.

================
Comment at: lib/Target/Mips/MipsInstrInfo.td:859
@@ -858,1 +858,3 @@
 
+def LONG_BRANCH_LUi : PseudoSE<(outs GPR32Opnd:$dst),
+  (ins brtarget:$tgt, brtarget:$baltgt), []>;
----------------
Ditto: add comments here?

================
Comment at: lib/Target/Mips/MipsLongBranch.cpp:14
@@ -13,5 +13,3 @@
 // FIXME:
 // 1. Fix pc-region jump instructions which cross 256MB segment boundaries.
 //===----------------------------------------------------------------------===//
----------------
Doesn't need a number if you remove point 2.

================
Comment at: lib/Target/Mips/MipsLongBranch.cpp:177
@@ -178,1 +176,3 @@
     // Compute size of MBB.
+    // FIXME: For inline asm instruction, GetInstSizeInBytes calls
+    // TargetInstrInfo::getInlineAsmLength.  For the MipsLongBranch
----------------
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.

================
Comment at: lib/Target/Mips/MipsLongBranch.cpp:273
@@ -269,7 +272,3 @@
 
-    int64_t TgtAddress = MBBInfos[TgtMBB->getNumber()].Address;
     unsigned BalTgtMBBSize = 5;
 
----------------
I think this isn't needed any more.  See other comment about the assert().

================
Comment at: test/CodeGen/Mips/longbranch.ll:6
@@ -5,3 +9,5 @@
 
-define void @foo1(i32 %s) nounwind {
 entry:
----------------
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?

================
Comment at: lib/Target/Mips/MipsMCInstLower.cpp:233
@@ -156,3 +232,3 @@
 
-  for (unsigned i = 0, e = MI->getNumOperands(); i != e; ++i) {
-    const MachineOperand &MO = MI->getOperand(i);
+  for (unsigned I = 0, E = MI->getNumOperands(); I != E; ++I) {
+    const MachineOperand &MO = MI->getOperand(I);
----------------
If you're doing style cleanups to existing code, it would be better to commit that as a separate change.

================
Comment at: lib/Target/Mips/MipsMCInstLower.cpp:200
@@ +199,3 @@
+                                      MCInst &OutMI) const {
+  if (MI->getOpcode() == Mips::LONG_BRANCH_LUi) {
+    lowerLongBranchLUi(MI, OutMI, Mips::LUi, MipsMCExpr::VK_Mips_ABS_HI);
----------------
Would this be better as a switch()?

================
Comment at: lib/Target/Mips/MipsMCInstLower.cpp:210
@@ +209,3 @@
+    lowerLongBranchADDiu(MI, OutMI, Mips::ADDiu,
+                             MipsMCExpr::VK_Mips_ABS_LO);
+    return true;
----------------
Nit: align indentation with "("

================
Comment at: lib/Target/Mips/MipsLongBranch.cpp:386
@@ -373,3 +385,3 @@
     assert(BalTgtMBBSize == BalTgtMBB->size());
     assert(LongBrMBB->size() + BalTgtMBBSize == LongBranchSeqSize);
   } else {
----------------
Should this become the following?
  assert(LongBrMBB->size() + BalTgtMBB->size() == LongBranchSeqSize);


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