This is an archive of the discontinued LLVM Phabricator instance.

MicroMIPS 16-bit unconditional branch instruction B
ClosedPublic

Authored by jkolek on Apr 25 2014, 9:18 AM.

Details

Summary

Implement microMIPS 16-bit unconditional branch instruction B.

Implemented 16-bit microMIPS unconditional instruction has real name B16, and B is an alias which expands to either B16 or BEQ according to the rules:
b 256 --> b16 256 # R_MICROMIPS_PC10_S1
b 12256 --> beq $zero, $zero, 12256 # R_MICROMIPS_PC16_S1
b label --> beq $zero, $zero, label # R_MICROMIPS_PC16_S1

Diff Detail

Event Timeline

jkolek updated this revision to Diff 8850.Apr 25 2014, 9:18 AM
jkolek retitled this revision from to MicroMIPS 16-bit unconditional branch instruction (B).
jkolek updated this object.
jkolek edited the test plan for this revision. (Show Details)
jkolek added reviewers: dsanders, matheusalmeida.
jkolek added subscribers: zoran.jovanovic, petarj.
dsanders edited edge metadata.Apr 30 2014, 7:24 AM

I haven't reviewed the actual code yet but I'm very concerned the about removals from the testcases. It looks like the implementation of the 16-bit branch encoding (with a PC10 reloc) has disabled the 32-bit branch encoding (with a PC16 reloc). Could you explain?

test/MC/Mips/micromips-branch-instructions.s
56

Why was this test removed? PC16 is still valid.

test/MC/Mips/micromips-branch16.s
61

Why was this test removed? PC16 is still valid

test/MC/Mips/micromips-diagnostic-fixup.s
3

I disagree with this change of error message. It's not wrong to be out of range for a PC10 if you are in range for a PC16.

test/MC/Mips/micromips-pc16-fixup.s
6 ↗(On Diff #8850)

Why did this change?

I'm aware that 'b foo' is really encoded as 'beq $0, $0, foo' but the canonical form should still be 'b foo'

This implementation of the 16-bit branch encoding B (with a PC10 reloc) has not disabled the 32-bit branch encoding BEQ (with a PC16 reloc), but what is different is that B is not anymore alias for the BEQ, but a real 16-bit instruction, and therefore when assembler reads 'b foo' it will generate PC10 reloc. Unfortunately there is a name collision for BEQ alias and 16-bit microMIPS unconditional branch instruction B.

test/MC/Mips/micromips-branch-instructions.s
56

Here I could change 'b 1332' to 'beq $0, $0, 1332', but that instruction is already here. 'b 1332' will generate 16-bit instruction encoding, which are placed in separate file.

test/MC/Mips/micromips-branch16.s
61

Since 'b bar' will generate R_MICROMIPS_PC10_S1 this i moved to the file micromips-branch10.s.

test/MC/Mips/micromips-diagnostic-fixup.s
3

Same as in test micromips-pc16-fixup.s, B has PC10 since it's not alias for BEQ anymore, but a real microMIPS instruction with 10 bit offset. I could change PC16 to PC10, or 'b foo' to 'beq $0, $0, foo'.

test/MC/Mips/micromips-pc16-fixup.s
6 ↗(On Diff #8850)

I have supposed that here PC16 is tested, so I changed 'b foo' to 'beq $0, $0, foo' since B is not anymore alias for the BEQ, but a real microMIPS 16-bit instruction, and therefore has PC10. Unfortunately there is a name collision for BEQ alias and 16-bit microMIPS unconditional branch instruction B.

We should probably add '.set micromips' to these tests. My first attempt at running them through GAS produced MIPS32R2 code :-).

Thanks for the explanation. At the moment, I'm still not convinced the test changes are right (I'll have a think about it) but it seems that the tests weren't right before this patch either. Given that the integrated assembler is aiming to be compatible with a subset of GAS, I think we need to assemble to the same instruction(s) it uses. I found that GAS always emits a sequence of instructions.

test/MC/Mips/micromips-branch-instructions.s
56

GAS segfaults on this test. I'm not sure why.

test/MC/Mips/micromips-branch16.s
61

GAS does something I don't understand here:

0:   9400fffe        lhu     zero,-2(zero)
                     0: R_MICROMIPS_PC16_S1  bar
4:   0c009483        jal     0x2520c
                     6: R_MICROMIPS_PC16_S1  bar
8:   fffe0c00        sdc3    $30,3072(ra)
c:   b483fffe        0xb483fffe
                     c: R_MICROMIPS_PC16_S1  bar
test/MC/Mips/micromips-diagnostic-fixup.s
3

When I ran this test through GAS I was expecting an error but it accepted it:

0:       9400fffe        lhu     zero,-2(zero)
                 0: R_MICROMIPS_PC16_S1  foo
4:       0c000101        jal     404 <foo-0xfbfc>
8:       01010101        0x1010101
test/MC/Mips/micromips-pc16-fixup.s
6 ↗(On Diff #8850)

GAS gives:

0:       9400fffe        lhu     zero,-2(zero)
                 0: R_MICROMIPS_PC16_S1  foo
4:       0c000101        jal     404 <foo-0xfbfa>
8:       01010101        0x1010101
jkolek updated this revision to Diff 9167.May 7 2014, 7:48 AM
jkolek edited edge metadata.

In this patch the implemented 16-bit microMIPS unconditional instruction has real name "B16", and "B" is an alias, that is expanded to either B16 or BEQ according to rules:
b 256 --> b16 256 # R_MICROMIPS_PC10_S1
b 12256 --> beq $zero, $zero, 12256 # R_MICROMIPS_PC16_S1
b label --> beq $zero, $zero, label # R_MICROMIPS_PC16_S1

Also, I have added the label "main" and directive ".set micromips" into newly created and some existing tests.

jkolek updated this revision to Diff 16091.Nov 12 2014, 7:56 AM
jkolek retitled this revision from MicroMIPS 16-bit unconditional branch instruction (B) to MicroMIPS 16-bit unconditional branch instruction B.
jkolek updated this object.
jkolek edited reviewers, added: vmedic, sstankovic; removed: matheusalmeida.
jkolek added a subscriber: Unknown Object (MLST).
jkolek updated this revision to Diff 16096.Nov 12 2014, 8:43 AM
jkolek updated this revision to Diff 17609.Dec 23 2014, 2:24 PM

Rebased patch, implemeted decoder method DecodeBranchTarget10MM() and added disassembler tests.

sstankovic added inline comments.Dec 26 2014, 7:05 AM
lib/Target/Mips/AsmParser/MipsAsmParser.cpp
1766

Why not use 16-bit nop here?

lib/Target/Mips/MCTargetDesc/MipsAsmBackend.cpp
221

Don't set FullSize here - add a case for fixup_MICROMIPS_PC10_S1 in the switch statement above.

224

Don't change microMipsLEByteOrder here - change the method needsMMLEByteOrder to return false for R_MICROMIPS_PC10_S1.

lib/Target/Mips/MCTargetDesc/MipsMCCodeEmitter.cpp
246

Same here.

250

Parameters are unaligned.

lib/Target/Mips/MCTargetDesc/MipsMCCodeEmitter.h
112

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

lib/Target/Mips/MipsLongBranch.cpp
507 ↗(On Diff #17609)

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.

test/MC/Mips/micromips-branch10.s
2 ↗(On Diff #17609)

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.

test/MC/Mips/micromips-branch16.s
0

Can you add support for printing this as "b bar"?

jkolek updated this revision to Diff 18083.Jan 13 2015, 5:42 AM

The files 'micromips-branch7.s', 'micromips-branch7.s' and 'micromips-branch10.s' are merged into 'micromips-branch-fixup.s'.

sstankovic accepted this revision.Jan 20 2015, 6:31 AM
sstankovic edited edge metadata.
This revision is now accepted and ready to land.Jan 20 2015, 6:31 AM
This revision was automatically updated to reflect the committed changes.
test/MC/Mips/micromips-branch16.s