This is an archive of the discontinued LLVM Phabricator instance.

MIPS64r6 LSA/DLSA instructions
ClosedPublic

Authored by jkolek on May 23 2014, 7:19 AM.

Details

Summary

Implemented *_DESC and *_ENC classes for MIPS64r6 LSA and DLSA instructions.

Diff Detail

Repository
rL LLVM

Event Timeline

jkolek updated this revision to Diff 9758.May 23 2014, 7:19 AM
jkolek retitled this revision from to MIPS64r6 LSA/DLSA instructions.
jkolek updated this object.
jkolek edited the test plan for this revision. (Show Details)
jkolek added reviewers: dsanders, vmedic.
jkolek added a subscriber: zoran.jovanovic.
dsanders edited edge metadata.May 23 2014, 7:38 AM

LSA/DLSA in MIPS32r6/MIPS64r6 are the same instructions as in the MSA ASE. Can you explain the need for a second copy rather than correcting the predicates of the pre-existing one?

jkolek updated this revision to Diff 9832.May 27 2014, 5:30 AM
jkolek edited edge metadata.

This patch implements support for LSA & DLSA instructions using predicates. This approach first includes all the MSA instructions in MIPS32r6/MIPS64r6 and then excludes all of them except LSA and DLSA. Maybe there is a better solution for this using predicates, but I didn't have a better idea. Zoran's suggestion is that we could reuse *_DESC and *_ENC classes from MSA, but to introduce new instruction definitions for MIPS32r6/MIPS64r6.

To be clear, I was only requesting an explanation for the seemingly redundant definition. I was guessing that the 32 feature bit limitation was likely to be the reason you couldn't follow the pattern used elsewhere (see below for details) in which case I would have requested a FIXME comment and approved it.

Your previous patch is better than the current one. The current one makes it impossible to use MSA on MIPS32r6/MIPS64r6.

See FeatureMips3_32 for an example of how to do a 'FeatureX || FeatureY' predicate. You define a feature that is the set of instructions in both MIPS32r6 and MSA (likewise for MIPS64r6 and MSA) and make that feature an implied bit for both MIPS32r6 and MSA. The snag is that this requires extra feature bits and tablegen currently asserts when there's more than 32. I have some plans for fixing this (either lifting the 32 feature limit, or adding support for an or condition) but it won't be a quick fix.

lib/Target/Mips/MipsMSAInstrInfo.td
3252 ↗(On Diff #9832)

It's a moot point since I've asked you to go back to the previous version of the patch but DLSA does not require the N64 ABI. It's requires 64-bit GPRs.

dsanders requested changes to this revision.Jun 2 2014, 2:40 AM
dsanders edited edge metadata.
This revision now requires changes to proceed.Jun 2 2014, 2:40 AM
jkolek updated this revision to Diff 10561.Jun 18 2014, 6:21 AM
jkolek edited edge metadata.

Previous version of patch, rebased.

You haven't answered the original question yet but I've just tried the FeatureMips3_32-style approach and we do hit the 32 feature bit limit. I know what needs to be done to fix that but I lack the time to do so at the moment. I therefore think it's best that we go ahead with this patch (with a minor change described below) and fix the 32 feature bit limit issue later.

Can you add a RUN line for mips32r6 and mips64r6 to test/CodeGen/Mips/msa/special.ll? E.g:

; RUN: llc -march=mips -mcpu=mips32r6 -mattr=+msa < %s | \
; RUN:   FileCheck %s --check-prefix=MIPS32
; RUN: llc -march=mips64 -mcpu=mips64r6 -mattr=+msa < %s | \
; RUN:   FileCheck %s --check-prefix=MIPS64

This will ensure that the two definitions don't cause problems for MSA on MIPS32r6/MIPS64r6.

With that change it will LGTM.

jkolek updated this revision to Diff 10566.Jun 18 2014, 7:44 AM
jkolek edited edge metadata.

I have added the RUN lines to test/CodeGen/Mips/msa/special.ll.

dsanders accepted this revision.Jun 18 2014, 9:14 AM
dsanders edited edge metadata.

LGTM

This revision is now accepted and ready to land.Jun 18 2014, 9:14 AM
jkolek closed this revision.Jun 20 2014, 2:36 AM
jkolek updated this revision to Diff 10687.

Closed by commit rL211346 (authored by zjovanovic).

jkolek edited edge metadata.Nov 18 2014, 5:53 AM
jkolek added a subscriber: Unknown Object (MLST).