This is an archive of the discontinued LLVM Phabricator instance.

[mips] [IAS] Add support for LAReg with identical source and destination register operands.
ClosedPublic

Authored by tomatabacu on Apr 30 2015, 3:22 AM.

Details

Summary

In this case, we're supposed to load the immediate in AT and then ADDu it with the source register and put it in the destination register.

Diff Detail

Event Timeline

dsanders requested changes to this revision.Jun 1 2015, 5:25 AM
dsanders edited edge metadata.
dsanders added inline comments.
lib/Target/Mips/AsmParser/MipsAsmParser.cpp
1757–1758

There's no need for UseAT. It's equivalent to ATReg == 0

1803

The 'UseAT ? ATReg : DstReg' is repeated a number of times. Please use a variable to represent the temporary register.

test/MC/Mips/mips-expansions.s
196

I think this should be:

addiu $8, $8, 20
This revision now requires changes to proceed.Jun 1 2015, 5:25 AM
tomatabacu updated this revision to Diff 27522.Jun 11 2015, 9:31 AM
tomatabacu edited edge metadata.

Addressed review comments.
Rebased (for Sean Bruno's convenience).

tomatabacu added inline comments.Jun 11 2015, 9:37 AM
lib/Target/Mips/AsmParser/MipsAsmParser.cpp
1757–1758

Fixed.

1803

Fixed.

test/MC/Mips/mips-expansions.s
196

I thought this was reported as a bug to the GAS developers and they said that they would switch to also use ORi.
Even if we decide to change it, it should be in a separate patch.

dsanders accepted this revision.Jun 12 2015, 3:05 AM
dsanders edited edge metadata.

LGTM with a nit

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

Nit: We don't need to use ATReg again so you can fold that ternary operator into the control flow above.

test/MC/Mips/mips-expansions.s
196

I'd forgotten about that, thanks for reminding me.

It's not exactly a bug. If I remember my conversation with Matthew correctly, there was some benefit to favoring addiu in the really old CPU's but that benefit no longer applies to anything in use today.

This revision is now accepted and ready to land.Jun 12 2015, 3:05 AM
tomatabacu updated this revision to Diff 27575.Jun 12 2015, 6:25 AM
tomatabacu edited edge metadata.

Moved ATReg inside the if block, as it is not used again after that.
Shortened the name of IntermediateDstReg to TmpReg.
Rebased on top of the new D9523, D9348 and D9366.
Removed unnecessary dependencies on other patches.

tomatabacu updated this revision to Diff 27926.Jun 18 2015, 3:51 AM

Rebased.

Daniel, is shortening IntermediateDstReg to TmpReg OK with you ?
You pointed that out in private, but since I ended up making the change, your position should be included in the public review.

LGTM

Daniel, is shortening IntermediateDstReg to TmpReg OK with you ?
You pointed that out in private, but since I ended up making the change, your position should be included in the public review.

I didn't mind either way. When we spoke, I was making a general comment that descriptive names are generally good but short, non-descriptive names aren't always bad. The obvious example is 'i', and 'j' for loop counters. Personally, I use TmpReg for trivial scratch registers and use more descriptive names when there are several of them (otherwise it starts to get confusing).

tomatabacu closed this revision.Jun 22 2015, 6:14 AM