Page MenuHomePhabricator

[llc+llvm-mc] Prefer applying -mabi to the triple where possible and allow Mips to take ABI information in the triple.
Needs ReviewPublic

Authored by sdardis on Jun 17 2016, 5:57 AM.

Details

Summary

The primary aim of this patch (and the clang counterpart) is to make it possible
to enable IAS by default for the N64 ABI (which is almost ready for it) without
also enabling it for the N32 ABI (which is definitely not ready). The same
change also lays the necessary ground work for fixing the N32 support such as
picking the right ELFCLASS and relocation encoding (N32 doesn't support the
3-relocs-in-1 encoding that N64 has).

The overall approach is to move away from the MCTargetOptions::ABIName way of
specifying the ABI (which until fairly recently was only in use by MIPS) and
move to specifying the ABI in the triple like other targets. Clang and API
users will then be required to specify the correct triple for the ABI they wish
to target.

In more detail:
Added the following environment values to llvm::Triple to explicitly
specify the ABI as part of the environment component of the triple:

  • ABI32, ABIN32, ABI64
  • AndroidABI32, AndroidABI64
  • GNUABI32, GNUABIN32, GNUABI64

ABI32/AndroidABI32/GNUABI32 are used internally to distinguish between the case
where O32 has been specified, and the case where the default ABI is implied.
This matters for triples such as mips64-linux-gnu where the default ABI is
context sensitive and could be any of the three main ABI's depending on
the environment. It is unlikely that these 'ABI32' suffixes will be seen in
GNU Triples.

In this patch, the MIPS backend will accept ABI's through either
MCTargetOptions::ABIName or the triple. A subsequent patch will restrict
this so that the MIPS backend only accepts ABI information from the triple
to avoid the potential for confusion caused by mismatches between the
triple and MCTargetOptions. At that point a corresponding clang patch will
be required to ensure proper usage and C++ API users who pass non-empty ABI
names will need to add something like:

Triple ABITriple = TT.getABIVariant(ABIName)
if (ABITriple.getArch() != Triple::UnknownArch) {
  TT = ABITriple;
  ABIName = ""
}

to their code. C-API users will be unaffected by this requirement since they
are currently unable to specify the ABI.

The test changes are as follows:
test/CodeGen/Mips/fp16-promote.ll

Changed gnueabi to gnu in the triple. EABI has been removed recently
because it was untested and had bitrotted.

test/MC/Mips/cpsetup.s

Made the OS component of the triple known since getABIVariant() needs to
know the OS to apply the ABI information to the triple.

test/MC/Mips/mips64-register-names-o32.s:

Switched daddiu's for addiu's since O32 prohibits the use of 64-bit GPRs.

See http://reviews.llvm.org/D20916 for an explanation as to why
MCTargetOptions::ABIName doesn't work for Mips.

Depends on D21466 and D21465

Patch By: Daniel Sanders

Diff Detail

Event Timeline

dsanders updated this revision to Diff 61085.Jun 17 2016, 5:57 AM
dsanders retitled this revision from to [llc+llvm-mc] Prefer applying -mabi to the triple where possible and allow Mips to take ABI information in the triple..
dsanders updated this object.
dsanders added a subscriber: llvm-commits.
dsanders updated this revision to Diff 61090.Jun 17 2016, 6:24 AM

Fixed a small rebase mistake that only became clear when a follow-up patch was applied

Are the new triples something we are inventing or are they supported
already by some tool. For example, can binutils be configured to
target mips64-linuxgnuabi64?

Cheers,
Rafael

Are the new triples something we are inventing or are they supported already by some tool.

A bit of both. Most of them are inventions for the LLVM internals. However, 'gnuabin32' and 'gnuabi64' are chosen to coincide with Debian's multiarch tuples (https://wiki.debian.org/Multiarch/Tuples, search for 'mips-linux-gnu' to find the mips examples) and will be used by the N32 port of Debian (if/when we do it) and is used by the new N64 port of Debian.

These triples are internal to LLVM and I'm expecting them to be (almost) invisible to the end user. Clang users will still use whatever triple and options they use today. Existing API users, existing LLVM-IR, lldb, llc, llvm-mc, etc. can also stick to their current triples if they wish but they won't be able to target N32 (which is the same position they're in at the moment). The reason I say 'almost' invisible is because the triples will show up in the 'target triple' statement of newly generated LLVM-IR. I also plan to have them appear in the output of 'clang --version' so that we can easily find out what the target for a given set of options is.

For example, can binutils be configured to target mips64-linux-gnuabi64?

On Debian, yes and it selects the appropriate behaviour to target the N64 port of Debian. Elsewhere, yes (because of a wildcard match 'linux-gnu*') and anything special about it compared to mips64-linux-gnu is specified in options to ./configure.

dsanders updated this revision to Diff 61784.Jun 24 2016, 6:32 AM

Refresh patch following the commit of r273557

dsanders updated this revision to Diff 63500.Jul 11 2016, 6:39 AM

Refreshed patch. This one is affected by the part of the llvm-dev thread about making targets consistent.
This patch only moves the Mips target to this mechanism at the moment. I'll develop the ARM/X86/other
patches separately and we can either commit them in sequence or fold them all into a single patch.

This patch only moves the Mips target to this mechanism at the moment. I'll develop the ARM/X86/other
patches separately and we can either commit them in sequence or fold them all into a single patch.

X86 doesn't need any changes because it doesn't support -target-abi (or -mabi in clang) at all.

PowerPC doesn't need any changes because the ABI only affects codegen.

That just leaves ARM since no other target is using -target-abi. I'm still confirming whether we need any changes to account for ARM.

So now you have a situation where there are two methods, that are both
happening at the same time, of passing the ABI to the backend. There needs
to be one.

So now you have a situation where there are two methods, that are both happening at the same time, of passing the ABI to the backend. There needs to be one.

That's not caused by this patch (or series), it's been the case since r224492. This patch just makes it obvious because a command line option is doing the substitution instead of the human writing the tests.

There are three camps today:

  • ARM/PowerPC pass the ABI in _both_ the triple and MCTargetOptions
  • Mips passes the ABI in the triple in some cases (O32, N64) and MCTargetOptions in other cases (N32).
  • X86/everyone-else pass the ABI in the triple only

My patch series started out trying to reduce this to:

  • ARM/PowerPC pass the ABI in _both_ the triple and MCTargetOptions
  • Mips/X86/everyone-else pass the ABI in the triple only

I'm currently trying to re-work the plan to reduce this further one of these three options:

  • ARM/PowerPC/Mips/X86/everyone-else pass the ABI in _both_ the triple and MCTargetOptions
  • ARM/PowerPC/Mips/X86/everyone-else pass the ABI in MCTargetOptions only (already ruled out because MCTargetOptions is not sufficient for ARM/X86/Mips)
  • ARM/PowerPC/Mips/X86/everyone-else pass the ABI in the triple only. (not ruled out yet but is significantly trickier than the first option)

The comments you're replying to are in line with aiming for the first of these three options.

I can't reach the end point in a single leap without breaking things or ending up with a truly massive patch. I need you to let me incrementally move towards it so that the trunk always works.


From: Eric Christopher [echristo@gmail.com]
Sent: 11 July 2016 18:18
To: reviews+D21467+public+827b718e7f297099@reviews.llvm.org; Daniel Sanders
Cc: llvm-commits@lists.llvm.org
Subject: Re: [PATCH] D21467: [llc+llvm-mc] Prefer applying -mabi to the triple where possible and allow Mips to take ABI information in the triple.

So now you have a situation where there are two methods, that are both happening at the same time, of passing the ABI to the backend. There needs to be one.

dsanders updated this revision to Diff 63673.Jul 12 2016, 6:07 AM

Changed Triple::getABIVariant() such that it returns a (possibly modified)
triple as well as a (possibly modified) ABI name. This allows it to handle
all three groups of target:

  • Those that pass ABI information in both the triple and MCTargetOptions (ARM/PowerPC).
  • Mips which passes the ABI in the triple in some respects (mips/mipsel => O32, mips64/mips64el => N32/N64) and in MCTargetOptions only in others (N32 vs N64).**
  • Those that that pass ABI information in the triple only (X86/everyone-else)
  • This is slightly different to the description I gave last night. Both are accurate but this one is more complete. The main point is that Mips is inconsistent.

At this point Mips is edging into the X86/everyone-else camp. A clang
patch (D21070) and another llvm patch (D21069) are needed to complete that
transition. This will leave us with two groups of targets (ARM/PowerPC, and
Mips/X86/everyone-else) at which point we can start reducing it down to a
single group.

Removed an N32 Android test from test/CodeGen/Mips/ehframe-indirect.ll since N32 isn't supported on Android.

@echristo: Does the latest version help resolve your concerns?

dsanders updated this revision to Diff 63677.Jul 12 2016, 8:05 AM

clang uses 'darwinpcs' which isn't mentioned in the backend. Added it to the list of ABI names.

dsanders updated this revision to Diff 64866.Jul 21 2016, 5:59 AM

Refresh and ping following upstream test updates and addition of Debian mips64el support.

dsanders updated this revision to Diff 64904.Jul 21 2016, 8:41 AM

Triple::isAndroid() and MipsABIInfo::computeTargetABI() now account for the
AndroidABI32 and AndroidABI64 enums. This prevents some breakage I noticed
while trying to make the clang driver mutate the triple instead of cc1/cc1as.

dsanders updated this revision to Diff 65915.Jul 28 2016, 5:01 AM

Refresh and ping

One last ping since I need to either commit this series next week or hand over to a colleague to continue it.

sdardis commandeered this revision.Aug 23 2016, 6:22 AM
sdardis added a reviewer: dsanders.

Taking over this patch series.

sdardis updated this revision to Diff 68980.Aug 23 2016, 6:23 AM
sdardis updated this object.
sdardis edited edge metadata.

Rebase + ping.

sdardis updated this revision to Diff 73052.Sep 30 2016, 7:54 AM

Refresh and ping.

sdardis updated this revision to Diff 73055.Sep 30 2016, 8:23 AM
sdardis edited edge metadata.

Correct failing test in diff.

sdardis updated this revision to Diff 76725.Nov 2 2016, 9:56 AM
sdardis edited edge metadata.

Rebase and ping.

dsanders resigned from this revision.Jul 18 2019, 6:56 PM