Page MenuHomePhabricator

[WIP][mips] Allow explicit ABI's in MIPS triples within LLVM.
AbandonedPublic

Authored by dsanders on Jun 2 2016, 9:10 AM.

Details

Reviewers
None
Summary

The primary aim of this patch 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.

TODO: Find out whether GNUX32/GNUEABI/GNUEABIHF are supposed to be exceptions

to the conditions in TargetLoweringBase.cpp and LegalizeDAG.cpp.

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.

Most of the test changes in this patch are simple -target-abi= to -mabi=
conversions but there are a few exceptions:
test/CodeGen/Mips/ehframe-indirect.ll:

Android does not support the N32 ABI. Switched the relevant test to
mips-unknown-linux-gnu instead.

test/CodeGen/Mips/fp16-promote.ll

Changed gnueabi to gnu in the triple. We have no EABI buildbots and no
'make check-all' tests (aside from this one) so I'm planning to remove it.

test/MC/Mips/cpload.s:

This test still had the old -mattr=+o32 and -mattr=+n64 options which
haven't been in use for a long time. Removed them.

test/MC/Mips/mips64extins.ll:

Converted this test to an assembler test since it's in test/MC

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

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

Why not use MCTargetOptions::ABIName?

Using MCTargetOptions::ABIName has some significant drawbacks for MIPS and
is likely to lead to further problems down the line.

One of the drawbacks is that to enable IAS by default for N64 we need to
make MCTargetOptions available to MipsMCAsmInfo which requires changing
both the public C++ API and the public C API
(LLVMCreateDisasmCPUFeatures() in particular). Furthermore, we would need
to make similar changes to the public C++ API to make MCTargetOptions
available to MipsMCCodeEmitter, MipsMCAsmBackend, and the MIPS
implementation of createMCSubtargetInfo() to be able to fix IAS support
for the N32 ABI. This then leads to tools like llvm-objdump needing to pass
an ABI name into the create*() functions or knowing when the backend will
ignore the values given. In my opinion, the plumbing needed for this approach
is difficult and produces a poor outcome.

Beyond the API changes, there are some odd inconsistencies that look like
they'll cause further problems in the future. For example, IRObjectFile not
specifying the ABI in its CollectAsmUndefinedRefs function can cause the
occasional unexpected undefined references for some inline assembly.
Another example is that that IRLinker won't consider O32/N32/N64 modules to
be link-incompatible with each other because it only compares the triples
stored in the modules. It therefore won't realize that the O32 LLVM-IR has
lowered the calling convention differently to the N64 LLVM-IR and link them
anyway to produce a bad output. Making IRLinker aware of the ABI would
require the ABI to be serialized in the LLVM-IR.

Diff Detail

Event Timeline

dsanders updated this revision to Diff 59396.Jun 2 2016, 9:10 AM
dsanders retitled this revision from to [WIP][mips] Allow explicit ABI's in MIPS triples within LLVM..
dsanders updated this object.
dsanders added a subscriber: llvm-commits.
dsanders updated this revision to Diff 59505.EditedJun 3 2016, 2:04 AM

Stop the backend determining the ABI from the CPU. The API user should work this out.
Make the calls to getABIVariant() unconditional.

dsanders updated this object.Jun 3 2016, 2:19 AM

Hi,

I'm not sure if the existing checks for T.getEnvironment() == Triple::GNU (which excludes GNUEABI/GNUABIHF/GNUX32) were correct or not in two places. Can someone confirm this one way or the other?

The first place is in LegalizeDAG.cpp:

static bool canCombineSinCosLibcall(SDNode *Node, const TargetLowering &TLI,
                                    const TargetMachine &TM) {
  if (!isSinCosLibcallAvailable(Node, TLI))
    return false;
  // GNU sin/cos functions set errno while sincos does not. Therefore
  // combining sin and cos is only safe if unsafe-fpmath is enabled.
  bool isGNU = Triple(TM.getTargetTriple()).getEnvironment() == Triple::GNU;
  if (isGNU && !TM.Options.UnsafeFPMath)
    return false;
  return true;
}

GNUEABI, GNUEABIHF, and GNUX32 won't cause isGNU to be true but the comment makes it sound like they should.

The second is in TargetLoweringBase.cpp:

if (TT.getEnvironment() == Triple::GNU) {
  Names[RTLIB::SINCOS_F32] = "sincosf";
  Names[RTLIB::SINCOS_F64] = "sincos";
  Names[RTLIB::SINCOS_F80] = "sincosl";
  Names[RTLIB::SINCOS_F128] = "sincosl";
  Names[RTLIB::SINCOS_PPCF128] = "sincosl";
}

There's nothing nearby that suggests that the omission of GNUEABI/GNUEABIHF/GNUX32 might be unintentional but if the LegalizeDAG.cpp one turns out to be unintentional then this one probably is too.

dsanders abandoned this revision.Jun 17 2016, 5:58 AM

This patch has been finished off and split into three smaller patches. These patches are D21465, D21466, and D21467