Page MenuHomePhabricator

[triple+llc+llvm-mc] Make triple and ABI name consistent
AbandonedPublic

Authored by atanasyan on Dec 11 2016, 6:17 AM.

Details

Summary

This patch is a reincarnation of D21467 and D22304 initially created by Daniel Sanders.

The aim of this patch is to provide ABI selected by a user to all parts of LLVM backend. It is especially important for MIPS because for this target ABI name might significantly change environment and even architecture specified by a triple.

The primary changes is introduced by this patch are new ABI specific for MIPS target and new Triple:: getABIVariant method. This method accepts ABI name and makes the triple and the ABI consistent.

Using only MCTargetOptions::ABIName as a source of ABI selected by a user 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

Repository
rL LLVM

Event Timeline

atanasyan updated this revision to Diff 81020.Dec 11 2016, 6:17 AM
atanasyan retitled this revision from to [triple+llc+llvm-mc] Make triple and ABI name consistent.
atanasyan updated this object.
atanasyan added reviewers: rafael, echristo, sdardis.
atanasyan set the repository for this revision to rL LLVM.
atanasyan added a subscriber: llvm-commits.
sdardis edited edge metadata.Jan 18 2017, 12:38 PM

This looks ok to me as a starting point to begin rely only on the triple for n32/n64. @echristo ?

include/llvm/ADT/Triple.h
208–209

I think for the moment, AndroidABI32, ABI*, and GNUABI32 are superfluous. We can infer enough and/or rely on clang to disambiguate the precise target.

atanasyan updated this revision to Diff 94631.Apr 9 2017, 10:22 PM
atanasyan edited the summary of this revision. (Show Details)
atanasyan added a reviewer: chandlerc.
  • Rebase the patch
  • Reduce diff with trunk

This patch was published four months ago. Any comments?

And one more ping. Does the silence mean implicit approval?

Hello,

I'd be interested to see this merged as well, but when I run the tests after applying the patch (onto 0d7672e plus my patch on https://reviews.llvm.org/D21465) I find that the following tests fail:

  • test/MC/Mips/macro-li.d.s
  • test/MC/Mips/macro-li.s.s

I think it is to do with lib/Target/Mips/MCTargetDesc/MipsMCAsmInfo.cpp lines 29-36

// FIXME: This condition isn't quite right but it's the best we can do until
//        this object can identify the ABI. It will misbehave when using O32
//        on a mips64*-* triple.
if ((TheTriple.getArch() == Triple::mipsel) ||
    (TheTriple.getArch() == Triple::mips)) {
  PrivateGlobalPrefix = "$";
  PrivateLabelPrefix = "$";
}

where we now should be able to access the ABI. But that file was not changed, the tests were broken by lib/Support/Triple.cpp at the end of getABIVariant:

// We have to keep the Arch and Environment in sync at the moment
// due to a false correlation between mips/mipsel and O32
// and mips64/mips64el and N64 that is required by the MIPS backend.
if (ABI == "o32")
  T = T.get32BitArchVariant();
else if (ABI == "n32" || ABI == "n64")
  T = T.get64BitArchVariant();

as the failing tests, for n32 and n64, specify the mips architecture (though with -mcpu=mips64 which seems to get ignored) with n32 and n64 ABI. But the above change forces mips64, thus the labels change from "$tmp" to ".Ltmp". However, I am not sure which behaviour we want: use dollar prefix for O32 ABIs only, in which case we need to change the test and MipsMCAsmInfo.cpp, or use dollar prefix for mips (not mips64) targets?

So some simple testing using gcc -S and

int main(void) {
  x: goto x;
}

as the simplest test-case with labels I could think of, seem to suggest that we should use $label for o32, and .Llabel for n32/n64, which is the current behaviour, but in that case perhaps the test cases should be changed so they align? I have included the way I would change those files, note that it also clarifies the difference between using two adjacent 32-bit floating-point registers, versus using one 64-bit floating-point register that the mips32r2 and newer support.

atanasyan abandoned this revision.Sep 21 2017, 6:55 AM

The N32 ABI support will be implemented as another series of patches/changes. Like rL313160, rL313231, rL313873 ...