This is an archive of the discontinued LLVM Phabricator instance.

[mips] [IAS] Add support for the B{L,G}{T,E}(U) branch pseudo-instructions.
ClosedPublic

Authored by tomatabacu on Mar 23 2015, 4:09 AM.

Details

Summary

This does not include support for the immediate variants of these pseudo-instructions.
Fixes llvm.org/PR20968.

Diff Detail

Event Timeline

tomatabacu updated this revision to Diff 22454.Mar 23 2015, 4:09 AM
tomatabacu retitled this revision from to [mips] [IAS] Add support for the B{L,G}{T,E}(U) branch pseudo-instructions..
tomatabacu updated this object.
tomatabacu edited the test plan for this revision. (Show Details)
tomatabacu added a reviewer: dsanders.
tomatabacu added subscribers: Unknown Object (MLST), emaste.
dsanders edited edge metadata.Mar 30 2015, 7:34 AM

I think there's a couple bugs in this. GAS and IAS behave differently for the following input:

t1:
blt $0, $0, t1
blt $0, $2, t1
blt $2, $0, t1
blt $2, $2, t1
blt $2, $3, t1
bltu $0, $0, t1
bltu $0, $2, t1
bltu $2, $0, t1
bltu $2, $2, t1
bltu $2, $3, t1
ble $0, $0, t1
ble $0, $2, t1
ble $2, $0, t1
ble $2, $2, t1
ble $2, $3, t1
bleu $0, $0, t1
bleu $0, $2, t1
bleu $2, $0, t1
bleu $2, $2, t1
bleu $2, $3, t1
bgt $0, $0, t1
bgt $0, $2, t1
bgt $2, $0, t1
bgt $2, $2, t1
bgt $2, $3, t1
bgtu $0, $0, t1
bgtu $0, $2, t1
bgtu $2, $0, t1
bgtu $2, $2, t1
bgtu $2, $3, t1
bge $0, $0, t1
bge $0, $2, t1
bge $2, $0, t1
bge $2, $2, t1
bge $2, $3, t1
bgeu $0, $0, t1
bgeu $0, $2, t1
bgeu $2, $0, t1
bgeu $2, $2, t1
bgeu $2, $3, t1

Assembling with each assembler then dissassembling the output with objdump shows several differences including missing/additional nops, different offsets (mostly caused by the nop differences), and different instructions. There's also a '...' in the disassembly for the IAS case and I'm not sure what that indicates. In each case the IAS is functionally equivalent, and in some cases is arguably better (e.g. 'blez $0, $0, t1' -> 'b t1'). However, we ought to be producing the same opcodes as GAS given the same input.


We should have a test that a warning is emitted when '.set noat' is in effect.

lib/Target/Mips/AsmParser/MipsAsmParser.cpp
211

We should rename this to avoid ambiguity between conditional branches and compact branches

1602–1612

This should probably be split into a separate patch which you can commit (with the braces nit fixed) without further review.

1605–1607

Nit: Added unnecessary braces.

lib/Target/Mips/MipsInstrInfo.td
1688

We should rename this to avoid ambiguity between conditional branches and compact branches

tomatabacu updated this revision to Diff 24307.Apr 23 2015, 9:06 AM
tomatabacu edited edge metadata.

Made expansions identical with GAS (will report mismatches as bugs in GAS):

blt  $0, $0 now expands to bltz $zero
ble  $0, $0 now expands to blez $zero
bge  $0, $0 now expands to bgez $zero
bgt  $0, $0 now expands to bgtz $zero
bgtu $0, $0 now expands to bnez $zero

Renamed to avoid possible confusion with compact branches.
Added error test for using conditional branch pseudos after .set noat.
Committed suggested change as rL235601.
Rebased on top of new D8480.

tomatabacu updated this object.May 1 2015, 10:02 AM
tomatabacu updated this revision to Diff 25759.May 14 2015, 2:31 AM

Rewrote comment about what I thought were buggy expansions in GAS (turns out I was wrong).
Rebased.

tomatabacu updated this revision to Diff 26133.May 20 2015, 2:57 AM

Added .set nomacro warning & test case.
Rebased.

dsanders accepted this revision.Jun 12 2015, 2:49 AM
dsanders edited edge metadata.

LGTM with a style nit.

One thing that we will need to address soon is that my while earlier test produces the same results for clang+IAS and gcc+GAS but clang+GAS produces completely different output. Presumably there's a clang-driver bug. Still, IAS is doing the right thing so this patch is good.

Also just an interesting observation, GAS only emits three 'branch ... is always true' warnings whereas IAS emits six. It seems GAS doesn't recognise $0 >= $0 and similar as being always true. IAS is doing the right thing here.

lib/Target/Mips/AsmParser/MipsAsmParser.cpp
2308–2441

Nit: Use early-return.

Rather than:

if (...) {
  ...
} else if (...) {
  ...
} else {
  ...
}
return false;

Use:

if (...) {
  ...
  return false;
}
if (...) {
  ...
  return false;
}
...
return false;
This revision is now accepted and ready to land.Jun 12 2015, 2:49 AM
tomatabacu updated this revision to Diff 27826.Jun 17 2015, 5:13 AM
tomatabacu edited edge metadata.

Addressed LGTM style nit.
Updated the ATReg-related code comment, as a result of this refactoring.

tomatabacu closed this revision.Jun 17 2015, 6:24 AM