This is an archive of the discontinued LLVM Phabricator instance.

llvm: Add support MIPS r6 Debian triples
ClosedPublic

Authored by wzssyqa on Aug 16 2018, 11:26 AM.

Details

Summary

Debian uses different triples for MIPS r6 and paths.
Here we use SubArch to determine whether it is r6,
if we found `r6' in CPU section of triple.

These new triples include:

mipsisa32r6-linux-gnu
mipsisa32r6el-linux-gnu
mipsisa64r6-linux-gnuabi64
mipsisa64r6el-linux-gnuabi64
mipsisa64r6-linux-gnuabin32
mipsisa64r6el-linux-gnuabin32

This patch depends on https://reviews.llvm.org/D51408

Diff Detail

Event Timeline

wzssyqa created this revision.Aug 16 2018, 11:26 AM

Could you add test cases to cover these changes?

include/llvm/ADT/Triple.h
96

The indentation change can be made by a separate patch / commit.

lib/Support/Triple.cpp
549

What kind of target triples covered by this expression "SubArchName.startswith("mips") && ..."?

wzssyqa updated this revision to Diff 161395.Aug 19 2018, 5:53 AM

Drop the modification by git-clang-format

wzssyqa updated this revision to Diff 161396.Aug 19 2018, 5:57 AM

a typo when replace.

wzssyqa updated this revision to Diff 162674.Aug 27 2018, 7:32 AM

add test case,

and fix llvm-mc to detect N32 and r6.

It would be nice to split this patch - the first one introduces new R6 target triples, the second one fixes N32 ABI handling.

lib/Support/Triple.cpp
401

Could you add test case for that code? To the TripleTest.cpp for example.

545

Does the SubArchName.startswith("mips") && (SubArchName.endswith("r6el") || SubArchName.endswith("r6")) expression cover "mipsisa32r6" and "mipsisa64r6" cases as well?

723

Could you add test case for that code? To the TripleTest.cpp for example.

lib/Target/Mips/MCTargetDesc/MipsABIInfo.cpp
58 ↗(On Diff #162674)

clang-format this line

lib/Target/Mips/MCTargetDesc/MipsAsmBackend.cpp
578 ↗(On Diff #162674)

Ditto

wzssyqa updated this revision to Diff 163069.Aug 29 2018, 6:11 AM
wzssyqa marked an inline comment as done.
wzssyqa edited the summary of this revision. (Show Details)Aug 29 2018, 6:13 AM
atanasyan added inline comments.Aug 31 2018, 11:55 PM
lib/Support/Triple.cpp
545

Is it possible to simplify this statement

if (SubArchName.startswith("mipsisa32r6") ||
      SubArchName.startswith("mipsisa64r6") ||
      (SubArchName.startswith("mips") &&
       (SubArchName.endswith("r6el") || SubArchName.endswith("r6"))))
    return Triple::MipsSubArch_r6;

and convert it to this form?

if (SubArchName.startswith("mips") && (SubArchName.endswith("r6el") || SubArchName.endswith("r6")))
    return Triple::MipsSubArch_r6;

Will we miss some target triples in that case?

atanasyan added inline comments.Sep 18 2018, 1:14 AM
lib/Support/Triple.cpp
545

By the way, if change the code as I suggested above, no one test from unittests/ADT/TripleTest.cpp failed. It means either the change is correct or the need to add more unit tests.

wzssyqa updated this revision to Diff 166211.Sep 19 2018, 7:05 PM

Looks good. Thanks. There are a couple notes only:

  • Let's extend existing triple unit tests and check that Triple::getSubArch() returns Triple::NoSubArch for non-R6 architectures.
  • I think it's redundant to add R6 target triple to so many "invalid.s" test cases. We check parsing of a target triple by unit tests. It's enough to take a few MIPS MC or CodeGen tests which use combination of non-R6 target triple and -mcpu=mips32r6 argument, remove -mcpu and replace target triple to R6 variant.
wzssyqa updated this revision to Diff 167236.Sep 26 2018, 7:48 PM
atanasyan accepted this revision.Sep 27 2018, 1:52 AM

LGTM. Thanks for the patch.

This revision is now accepted and ready to land.Sep 27 2018, 1:52 AM
This revision was automatically updated to reflect the committed changes.