This is an archive of the discontinued LLVM Phabricator instance.

[mips] Absolute value macro expansion
ClosedPublic

Authored by obucina on Jan 19 2016, 9:39 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

obucina updated this revision to Diff 45276.Jan 19 2016, 9:39 AM
obucina retitled this revision from to [mips] Absolute value macro expansion.
obucina updated this object.
obucina added reviewers: zoran.jovanovic, dsanders.
obucina added a subscriber: llvm-commits.
dsanders accepted this revision.Jan 22 2016, 6:30 AM
dsanders edited edge metadata.

LGTM with a couple changes

lib/Target/Mips/AsmParser/MipsAsmParser.cpp
3537 ↗(On Diff #45276)

This should be a nop when both registers are the same. Assembling

a: abs $2, $3
b: abs $2, $2
c:

gives:

00000000 <a>:
   0:	04610002 	bgez	v1,c <b>
   4:	00601021 	move	v0,v1
   8:	00031022 	neg	v0,v1

0000000c <b>:
   c:	04410002 	bgez	v0,18 <c>
  10:	00000000 	nop
  14:	00021022 	neg	v0,v0

00000018 <c>:
	...
3539 ↗(On Diff #45276)

This nop should be deleted. Assuming you tested this the same way I did (by assembling then disassembling) then the nop you saw was padding to round off the section size.

lib/Target/Mips/MipsInstrInfo.td
1811–1812 ↗(On Diff #45276)

Just a spelling nit: Could you make it obvious that it's a macro in the name? I'm thinking ABSMacro or ABSPseudo.

We ought to tidy up the existing names at some point but that's for another patch.

test/MC/Mips/macro-abs.s
5–9 ↗(On Diff #45276)

Could you test 'abs $4, $4' too?

This revision is now accepted and ready to land.Jan 22 2016, 6:30 AM
obucina updated this revision to Diff 46001.Jan 26 2016, 8:54 AM
obucina edited edge metadata.

New diff contains suggested changes

obucina marked 4 inline comments as done.Jan 26 2016, 8:54 AM
This revision was automatically updated to reflect the committed changes.