[PATCH] MicroMIPS 16-bit unconditional branch instruction B

[Sasa Stankovic]

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:1780
@@ +1779,3 @@
+    MCInst NopInst;
+    NopInst.setOpcode(Mips::SLL);
+    NopInst.addOperand(MCOperand::CreateReg(Mips::ZERO));
----------------
Why not use 16-bit nop here?

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsAsmBackend.cpp:219
@@ +218,3 @@
+  if ((unsigned) Kind == Mips::fixup_MICROMIPS_PC10_S1) {
+    // We have 16-bit unconditional branch instruction.
+    FullSize = 2;
----------------
Don't set FullSize here - add a case for fixup_MICROMIPS_PC10_S1 in the switch statement above.

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsAsmBackend.cpp:222
@@ +221,3 @@
+    // We don't want microMIPS specific little-endian byte ordering
+    // for 16-bit instructions.
+    microMipsLEByteOrder = false;
----------------
Don't change microMipsLEByteOrder here - change the method needsMMLEByteOrder to return false for R_MICROMIPS_PC10_S1.


================
Comment at: lib/Target/Mips/MCTargetDesc/MipsMCCodeEmitter.cpp:246
@@ +245,3 @@
+/// getBranchTargetOpValue - Return binary encoding of the microMIPS 10-bit
+/// branch target operand. If the machine operand requires relocation, record
+/// the relocation and return zero.
----------------
Same here.

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsMCCodeEmitter.cpp:250
@@ +249,3 @@
+getBranchTargetOpValueMMPC10(const MCInst &MI, unsigned OpNo,
+                         SmallVectorImpl<MCFixup> &Fixups,
+                         const MCSubtargetInfo &STI) const {
----------------
Parameters are unaligned.

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsMCCodeEmitter.h:112
@@ +111,3 @@
+  // getBranchTargetOpValue - Return binary encoding of the microMIPS 10-bit
+  // branch target operand. If the machine operand requires relocation, record
+  // the relocation and return zero.
----------------
The method name in comment doesn't match the name in code. I think that you don't need to specify method names in comments (at least I received such comment for one of my patches).


================
Comment at: lib/Target/Mips/MipsLongBranch.cpp:507
@@ +506,3 @@
+      if (IsMicroMips && !ForceLongBranch && I->Br->isUnconditionalBranch() &&
+          isInt<10>(computeOffset(I->Br) / ShVal)) {
+        if (buildMMUncondBranchInst(*I))
----------------
If I understand this correctly, this code replaces b (16-bit offset branch) with b16 (10-bit offset branch). But this is not what MipsLongBranch is supposed to do. In MipsLongBranch you should add code that checks whether the offset for b16 instructions fits in 10 bits, and if not, either replaces b16 with b or expands b16 to full long branch sequence, depending on the offset.

I think the proper place for replacing b with b16 would be some optimization pass run before MipsLongBranch (or you can generate b16 during instruction selection whenever you can). MipsLongBranch should only be the final check that offsets for b16 fit in 10-bit fields.


================
Comment at: test/MC/Mips/micromips-branch10.s:2
@@ +1,3 @@
+# RUN: llvm-mc %s -triple=mipsel-unknown-linux -show-encoding \
+# RUN: -mattr=micromips | FileCheck %s -check-prefix=CHECK-FIXUP
+# RUN: llvm-mc %s -filetype=obj -triple=mipsel-unknown-linux \
----------------
This file and the file micromips-branch16.s have identical RUN lines. I think you can merge these two files, and name the resulting file (for example)  micromips-branch-fixup.s, since both files check the relocations for micromips branches.

================
Comment at: test/MC/Mips/micromips-branch16.s:9
@@ -8,2 +8,3 @@
 # for relocations.
 #------------------------------------------------------------------------------
+# CHECK-FIXUP: beq $zero, $zero, bar # encoding: [A,0x94'A',0x00,0x00]
----------------
Can you add support for printing this as "b bar"?

http://reviews.llvm.org/D3514

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/