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

Mark Seaborn mseaborn at chromium.org
Sun Apr 27 10:23:51 PDT 2014


LGTM

================
Comment at: lib/Target/Mips/Mips64InstrInfo.td:243
@@ +242,3 @@
+// explanation.
+def LONG_BRANCH_LUi64 : PseudoSE<(outs GPR64Opnd:$dst),
+  (ins brtarget:$tgt, brtarget:$baltgt), []>;
----------------
Can you add a comment like this:
// Expands to: lui64 $dst, %highest($tgt - $baltgt)

================
Comment at: lib/Target/Mips/Mips64InstrInfo.td:246
@@ +245,3 @@
+
+def LONG_BRANCH_DADDiu : PseudoSE<(outs GPR64Opnd:$dst),
+  (ins GPR64Opnd:$src, brtarget:$tgt, brtarget:$baltgt), []>;
----------------
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.

================
Comment at: lib/Target/Mips/MipsInstrInfo.td:930
@@ +929,3 @@
+// explanation.
+def LONG_BRANCH_LUi : PseudoSE<(outs GPR32Opnd:$dst),
+  (ins brtarget:$tgt, brtarget:$baltgt), []>;
----------------
Can you add a comment like:
// Expands to: lui $dst, %hi($tgt - $baltgt)

================
Comment at: lib/Target/Mips/MipsInstrInfo.td:933
@@ +932,3 @@
+
+def LONG_BRANCH_ADDiu : PseudoSE<(outs GPR32Opnd:$dst),
+  (ins GPR32Opnd:$src, brtarget:$tgt, brtarget:$baltgt), []>;
----------------
Similarly:
// Expands to: addiu $dst, $src, %lo($tgt - $baltgt)

================
Comment at: lib/Target/Mips/MipsMCInstLower.cpp:171
@@ +170,3 @@
+  MCOperand Reg = LowerOperand(MI->getOperand(0));
+  if (Reg.isValid())
+    OutMI.addOperand(Reg);
----------------
When would this be false?  Should this be an assertion instead, or should this check be omitted?

================
Comment at: lib/Target/Mips/MipsMCInstLower.cpp:189
@@ +188,3 @@
+
+    if (Reg.isValid())
+      OutMI.addOperand(Reg);
----------------
Same here

================
Comment at: lib/Target/Mips/MipsMCInstLower.cpp:218
@@ +217,3 @@
+      lowerLongBranchADDiu(MI, OutMI, Mips::DADDiu, MCSymbolRefExpr::VK_Mips_ABS_HI);
+    else
+      lowerLongBranchADDiu(MI, OutMI, Mips::DADDiu, MCSymbolRefExpr::VK_Mips_ABS_LO);
----------------
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");


================
Comment at: test/CodeGen/Mips/longbranch.ll:3
@@ +2,3 @@
+; RUN: llc -march=mipsel -force-mips-long-branch -O3 < %s \
+; RUN:  | FileCheck %s -check-prefix=O32
+; RUN: llc -march=mips64el -mcpu=mips4 -mattr=n64 -force-mips-long-branch -O3 \
----------------
Nit: can you indent continuation lines by 2 or 4 spaces, rather than 1, for readability?

================
Comment at: test/CodeGen/Mips/longbranch.ll:53
@@ +52,3 @@
+
+; O32:        addiu   $sp, $sp, -8
+; O32:        sw      $ra, 0($sp)
----------------
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.

================
Comment at: test/CodeGen/Mips/longbranch.ll:54
@@ +53,3 @@
+; O32:        addiu   $sp, $sp, -8
+; O32:        sw      $ra, 0($sp)
+; O32:        lui     $1, %hi(($[[BB2:BB[0-9_]+]])-($[[BB1:BB[0-9_]+]]))
----------------
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.)

http://reviews.llvm.org/D3089






More information about the llvm-commits mailing list