This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC][Altivec] Add vmr extended mnemonic
ClosedPublic

Authored by brunoalr on Jan 25 2017, 10:20 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

brunoalr created this revision.Jan 25 2017, 10:20 AM
brunoalr planned changes to this revision.EditedJan 25 2017, 10:27 AM

Will add nemanjai's patch on mfvrd.

brunoalr updated this revision to Diff 85783.EditedJan 25 2017, 10:46 AM
brunoalr edited the summary of this revision. (Show Details)

mfvrd with VR registers is an alias to mfvsrd with VSR registers.

Patch by nemanjai

nemanjai added inline comments.Jan 26 2017, 4:04 AM
lib/Target/PowerPC/PPCInstrVSX.td
1450 ↗(On Diff #85783)

@hfinkel Do you think this is an acceptable way to get this support implemented? Or do you have a preferred suggestion? Maybe some custom handling of the register operand (or even the mnemonic itself) in the Asm parser/printer or something along those lines...

Once we decide how we want to approach this, we can add a test case.

hfinkel edited edge metadata.Jan 26 2017, 6:19 AM

mfvrd with VR registers is an alias to mfvsrd with VSR registers.

Patch by nemanjai

Why did you fold these together? Can we just do the vor -> vmr alias, and then worry about the mfvrd change separately? The vor -> vmr LGTM, so if you can pull these apart, please commit that part.

lib/Target/PowerPC/PPCInstrVSX.td
1450 ↗(On Diff #85783)

One of these should be mfvsrd?

mfvrd with VR registers is an alias to mfvsrd with VSR registers.

Patch by nemanjai

Why did you fold these together? Can we just do the vor -> vmr alias, and then worry about the mfvrd change separately? The vor -> vmr LGTM, so if you can pull these apart, please commit that part.

Yes, we can do them separately. I'll make the necessary changes to this diff. I don't have commit permissions yet, but I'll prepare the vor -> vmr diff.

brunoalr retitled this revision from [PowerPC][Altivec] Add some extended mnemonics to [PowerPC][Altivec] Add vmr extended mnemonics.Jan 26 2017, 6:25 AM
brunoalr retitled this revision from [PowerPC][Altivec] Add vmr extended mnemonics to [PowerPC][Altivec] Add vmr extended mnemonic.
nemanjai edited edge metadata.Jan 26 2017, 6:26 AM

Why did you fold these together? Can we just do the vor -> vmr alias, and then worry about the mfvrd change separately? The vor -> vmr LGTM, so if you can pull these apart, please commit that part.

Sorry, this is probably my fault. They needed these two extended mnemonics and I recommended they publish just one patch for them.
I'll commit the vor -> vmr patch. @brunoalr please post the mfvrd one separately.

nemanjai added inline comments.Jan 26 2017, 6:32 AM
lib/Target/PowerPC/PPCInstrVSX.td
1450 ↗(On Diff #85783)

The asm string that we want to match when parsing asm is mfvrd. However, doing so will produce an instruction with a VR input. So if we also use this alias for printing, we'll print $XT + 32. That's why the zero for the Emit flag.

In a way, this is a hack for the InstAlias class. We need to read the input and treat it as a different register class (with different numbering).

Why did you fold these together? Can we just do the vor -> vmr alias, and then worry about the mfvrd change separately? The vor -> vmr LGTM, so if you can pull these apart, please commit that part.

Sorry, this is probably my fault. They needed these two extended mnemonics and I recommended they publish just one patch for them.
I'll commit the vor -> vmr patch. @brunoalr please post the mfvrd one separately.

@nemanjai here: https://reviews.llvm.org/D29177

brunoalr updated this revision to Diff 86072.Jan 27 2017, 10:30 AM
  • Remove mfvrd portion
brunoalr updated this revision to Diff 86076.Jan 27 2017, 10:51 AM
  • Add vmr tests and remove mfvrd leftover code
brunoalr added a comment.EditedJan 27 2017, 10:53 AM

@hfinkel I updated this diff to include only the vmr portion and added a test here as well

@nemanjai @kbarton please continue the discussion on mfvrd here: D29177

Gently pinging :)

nemanjai accepted this revision.Jan 31 2017, 5:46 AM

Will commit this as we're all in agreement on the part that remains in this patch now.

This revision is now accepted and ready to land.Jan 31 2017, 5:46 AM
This revision was automatically updated to reflect the committed changes.