Page MenuHomePhabricator

Initial MIPS IV support. Separate out the is-MIPS64-ISA checks from is-64-bit-MIPS tests.
AbandonedPublic

Authored by dsanders on Feb 14 2014, 7:34 AM.

Details

Reviewers
sdardis
seanbruno
Summary

Patch by David Chisnall

Diff Detail

Event Timeline

I've discovered some important background while reviewing this patch. It seems that isMips64() predicates have been used incorrectly as a synonym for various things that aren't MIPS64 but are closely related to it. In particular some of it's uses ought to be checking for the use of the N32 or N64 ABI's rather than tests for MIPS64. I'm happy to sort this out. I mention it because it is likely to affect your patch.

I have mixed opinions about the IsGP64bit feature bit. I see your point that many of the isMips64() conditions are tests of the register size. On the other hand, the newer 64-bit cores are supersets of the older ones so we could also change the test for isMips3(). On balance, I think I'm in favour of a specific IsGP64bit feature bit that is set along with the architecture bit.

Lastly, it needs a basic test case to test that -arch=mips4 exists and functions. An empty assembly file that uses llvm-readobj to test the ELF e_flags would do the trick.

lib/Target/Mips/AsmParser/MipsAsmParser.cpp
217–218

We should rename this function to isGP64bit(), however we probably also need to keep isMips64() to guard instructions code paths that use instructions that were added in MIPS64.

There seem to be two pre-existing mistakes involving this function which need fixing (either of us can do this, I hope to be able to get to it next week).
The first is that the use of this function in expandLoadAddressSym() needs to be changed to isMips4() since it's guarding instructions rather than register width. It should probably end up as isMips3() once we support MIPS-III, we can either have an isMips3() that really tests for MIPS-IV for now, or we can just use a isMips4().

The second is that both uses of this function in matchCPURegisterName() are really testing for either N32 or N64 rather than for MIPS64.

lib/Target/Mips/MipsAsmPrinter.cpp
656–657 ↗(On Diff #7132)

The predicate here should be isMips4() which should test for FeatureMips4

lib/Target/Mips/MipsISelLowering.cpp
204–205

I generally agree with the use of IsGP64bit but we may need to hang on to HasMips64 as well.

390

This is a pre-existing issue, but this looks wrong. I'm told that in GCC, functions are 4-byte aligned for in O32, N32, and N64 ABI's. I'll look into this.

1519

I need to find out if this is the right predicate. I'm not sure if the use of MO_GOT_OFST is related to 64-bit targets in general, >=MIPS64 specifically, or ABI's.

What ever the outcome of that investigation, the parameter in the declaration and body will need to be renamed to match the new meaning.

Hi David,

I've put the first patch onto phabricator so that I can comment on it (I don't have a github account). I hope that's ok with you.

Most of the issues are related to pre-existing mistakes in the use of MIPS64 predicates which I'm happy to sort out. Other than that, there's a few minor things to correct.

I'm going to look into this branch today and tomorrow. Are you ok with me directly committing things that I'm happy with?

-----Original Message-----
From: Dr D. Chisnall [mailto:dc552@hermes.cam.ac.uk] On Behalf Of David
Chisnall
Sent: 20 March 2014 14:58
To: reviews+D2808+public+3ac3d123901a7937@llvm-reviews.chandlerc.com
Cc: Daniel Sanders
Subject: Re: [PATCH] Initial MIPS IV support. Separate out the is-MIPS64-ISA
checks from is-64-bit-MIPS tests.

Hi Daniel,

dsanders updated this revision to Unknown Object (????).Mar 25 2014, 6:58 AM
  • Initial MIPS IV support.
  • Rename GP64 check.
dsanders updated this revision to Unknown Object (????).Mar 25 2014, 8:23 AM

Rebased after fixing pre-existing issues (D3175, D3177, and D3178) found in review.

seanbruno requested changes to this revision.Sep 26 2016, 6:57 AM
seanbruno added a reviewer: seanbruno.
seanbruno added a subscriber: seanbruno.

I suspect, due to age, that this review needs to be recreated completely? Or is it something that should be looked into further?

This revision now requires changes to proceed.Sep 26 2016, 6:58 AM
sdardis edited edge metadata.Feb 1 2017, 8:12 AM

I'm not sure why this was left open, perhaps it was simply forgotten.

rL205530 , rL205968, rL206183, rL206185 contain these changes and various bits of follow on work.

I'm not sure why this was left open, perhaps it was simply forgotten.

rL205530 , rL205968, rL206183, rL206185 contain these changes and various bits of follow on work.

Yeah, this series of reviews seem to be outdated regardless.

I would probably just close them out unless dsanders has any objections.

I think most of this was committed. I remember fixing a few merge conflicts where upstream had the wrong check, at least...

dsanders abandoned this revision.Jul 18 2019, 6:59 PM