Page MenuHomePhabricator

[Mips][ABI]The ELF container needs to depend on the ABI rather than the target triple.
Needs ReviewPublic

Authored by vmedic on Dec 2 2014, 1:15 AM.

Details

Reviewers
echristo
dsanders
Summary

At the moment Mips back-end is deciding between ELF32/64 output based on target triple. We should instead use ABI which should be passed as -target-abi command line option. This is the patch that has been created on the bases of http://reviews.llvm.org/D6092 and updated to the top of the tree with no functional changes, subscribers added as requested in initial patch. It is still missing the test files, but we'd like to keep it short at the time until the remaining issues are solved. We still looking where would it be the best to initialize and store the ABI info value so that it's accessible from codegen, the integrated assembler, and the disassembler. Also, selectMipsCPU is currently a member of MipsABIInfo class which is not a best logical place for it, but it is placed there to be visible to all calling classes to avoid code multiplication.

Diff Detail

Event Timeline

vmedic updated this revision to Diff 16803.Dec 2 2014, 1:15 AM
vmedic retitled this revision from to [Mips][ABI]The ELF container needs to depend on the ABI rather than the target triple..
vmedic updated this object.
vmedic edited the test plan for this revision. (Show Details)
vmedic added reviewers: dsanders, sstankovic.
vmedic added subscribers: rafael, Unknown Object (MLST).

Daniel's comments from original issue(6092):
Thanks for working on this. It's very important that we stop our triples from affecting our targets behaviour (except in so far as they affect default option values).

I'm still going through the patch but there's one big thing I'm concerned about. We seem to be repeating ourselves quite a lot, so far I've counted five classes holding MipsABIInfo objects (one of which is cached from another object), and six places where it is (at least partially) initialized. At the moment, I'm not sure where best initialize and store the value so that it's accessible from codegen, the integrated assembler, and the disassembler. Perhaps we should be arranging for llc/clang/llvm-mc/etc. to pass in the ABI name through a target-independent API?
Rafael, do you have any suggestions?

Also, this patch needs clang-formatting and rebasing to top-of-tree. The rebase is needed because I've committed the MipsABIInfo patch you've built on top of and you seem to have some conflicts with it.

dsanders edited edge metadata.Dec 9 2014, 7:44 AM

(Please include the context as described at http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface).

I'd like to move MipsABIInfo.{cpp,h} in a separate patch if that's ok with you. This will make it easier to see what the changes are made by this patch.

so far I've counted five classes holding MipsABIInfo objects

I've looked into this properly and I believe we can reduce this a bit but not as much as I'd hoped. At the moment this patch initializes a MipsABIInfo object in:

  • MipsAsmParser::MipsAsmParser()
  • MipsDisassemblerBase::MipsDisassemblerBase()
  • MipsAsmBackend::createObjectWriter()
  • MipsRegInfoRecord::MipsRegInfoRecord()
  • MipsTargetELFStreamer::MipsTargetELFStreamer()
  • MipsSubtarget::MipsSubtarget()

MipsAsmParser doesn't really need it's own copy of the MipsABIInfo object. It could access it via the target streamers instead. To do this, we would have to move the initialization from MipsTargetELFStreamer::MipsTargetELFStreamer() into createMCStreamer() and pass it into both MipsTargetELFStreamer and MipsTargetAsmStreamer. Then MipsAsmParser can get the ABI with something like getTargetStreamer().getABI().

MipsRegInfoRecord doesn't really need to know the ABI. It only needs to know whether it should emit the record as part of .MIPS.options or .reginfo. We should therefore pass this information in to the MipsRegInfoRecord constructor (from MipsTargetELFStreamer which in turn got it from createMCStreamer) and remove the MipsABIInfo object.

The single check for N64 inside MipsDisassemblerBase's subclasses is actually wrong. It should be testing for FeatureGP64bit. Once this is fixed, this class won't need to know the ABI.

These refactors (which should be done in separate patches) leave us with these three initializations.

  • MipsAsmBackend::createObjectWriter()
  • createMCStreamer()
  • MipsSubtarget::MipsSubtarget()

I don't think it's possible to eliminate any more than that. With the constructor change as well (see below), the implementation will end up inside the MipsABIInfo constructor so this isn't too bad.

Do you mind doing those refactors before this patch?

lib/Target/Mips/MCTargetDesc/MipsABIInfo.cpp
56–66 ↗(On Diff #16803)

All the uses of MipsABIInfo use the following pattern:

MipsABIInfo ABI; // <- Default constructor reads the TargetABI option
if (!ABI.IsKnown())
  ABI = MipsABIInfo(IsGP64Bit);

sometimes this is split across functions but we never use a MipsABIInfo until we've done the if-statement.

Rather than having two constructors like this, I think it would be best to merge them into one like so:

MipsABIInfo::MipsABIInfo(bool Is64bit) {
  ThisABI = StringSwitch<ABI>(TargetABI.getValue())
    .Cases("32", "o32", ABI::O32)
    .Case("n32", ABI::N32)
    .Cases("64", "n64", ABI::N64)
    .Default(ABI::Unknown);
  if (!IsKnown())
    ThisABI = Is64Bit ? ABI::N64 : ABI::O32;
}

This makes it easy to see that it is impossible to use a partially constructed object.

lib/Target/Mips/MCTargetDesc/MipsAsmBackend.cpp
145–155

See comment on MipsABIInfo constructor.

157

We should be testing for properties of the ABI rather than the ABI itself. E.g. ABIInfo.UsesELF64Objects() rather than ABIInfo.IsN64().

The existing places where we test for the ABI need updating to this style but that's for another patch-series.

lib/Target/Mips/MCTargetDesc/MipsTargetStreamer.cpp
344–346 ↗(On Diff #16803)

See comment in MipsABIInfo constructor

lib/Target/Mips/MipsSubtarget.cpp
130–131 ↗(On Diff #16803)

See comment on MipsABIInfo constructor

lib/Target/Mips/MipsSubtarget.h
48 ↗(On Diff #16803)

This comment is wrong, it's the selected ABI. Please change it back.

49 ↗(On Diff #16803)

I don't see a need to rename this member.

lib/Target/Mips/MipsTargetStreamer.h
110–112 ↗(On Diff #16803)

Indendation should be spaces.

vmedic updated this revision to Diff 17632.EditedDec 25 2014, 3:24 AM
vmedic edited edge metadata.

Patch refactored according to previous comments.

Thanks for doing those refactors. There's quite a few comments below. Many are nits but there's a couple important ones.

As you mentioned when we spoke off-list, we still need test cases for this. An assembler test that verifies that mips/mips64 triples have no effect on the ELF32/ELF64-ness of the ELF output should do.

include/llvm/MC/MCParser/MCAsmParserExtension.h
68–71 ↗(On Diff #17632)

This new function isn't needed (deleting it doesn't break the build) and just calls the existing getStreamer() above after doing a dangerous cast.

include/llvm/MC/MCStreamer.h
233–236 ↗(On Diff #17632)

Deleting this doesn't break the build and that cast is dangerous.

lib/Target/Mips/AsmParser/MipsAsmParser.cpp
97–100 ↗(On Diff #17632)

This does seem to be needed but the cast is dangerous, we need to find another way.

It looks like we're going to have to cache the MipsABIInfo object from the target streamer during the MipsAsmParser constructor. Fortunately, it shouldn't change during execution.

334–335 ↗(On Diff #17632)

I don't see why this cast is needed.

lib/Target/Mips/MCTargetDesc/MipsABIInfo.cpp
50–55 ↗(On Diff #17632)

You'll need to resolve a conflict with the 'target-abi' option Eric Christopher added in r224492. It's normally accessed through the TargetOptions class.

I tried a couple quick fixes but didn't successfully get 'make check' passing.

56 ↗(On Diff #17632)

Nit: Variables normally start with a capital

lib/Target/Mips/MCTargetDesc/MipsABIInfo.h
30 ↗(On Diff #17632)

Nit: Variables normally start with capitals.

lib/Target/Mips/MCTargetDesc/MipsAsmBackend.cpp
145–150

This should probably be in MipsABIInfo just to keep it near to MipsABIInfo::selectMipsCPU(). We ought to find a proper home for both at some point.

409

Nit: You've added an unnecessary blank line here

lib/Target/Mips/MCTargetDesc/MipsELFObjectWriter.cpp
13 ↗(On Diff #17632)

I don't think you need this include.

lib/Target/Mips/MCTargetDesc/MipsMCTargetDesc.cpp
117–125 ↗(On Diff #17632)

These will both do the right thing (at the moment) since it only matters for ELF output but I'm not happy with the idea that the ABI can be different between ELF and Assembly output, nor that it can be O32 when we are really doing N32/N64.

lib/Target/Mips/MipsSubtarget.cpp
209 ↗(On Diff #17632)

Nit: You seem to have added a blank line here. Intentional? The context is missing so I can't tell if it's the end of the file or next to another blank line.

vmedic added inline comments.Jan 12 2015, 6:30 AM
lib/Target/Mips/MCTargetDesc/MipsABIInfo.cpp
50–55 ↗(On Diff #17632)

It seems to me that the patch does the same thing we do, only it puts the flag info TargetOptions and accesses trough TargetMachine class, while we are putting it into MipsABIInfo class. I'll try to see if we can use their implementation in our code.

vmedic updated this revision to Diff 20271.Feb 19 2015, 3:52 AM
vmedic edited reviewers, added: echristo; removed: sstankovic.
vmedic removed a subscriber: rafael.

As suggested by Eric, ABI info is taken from MCTargetOptions which is passed to createAsmBackend method. Definitions of these methods for all targets have been extended with this parameter to keep the build sanity. For the Mips target ABI info is extracted to create elf32/64 object file. Also, the way that extracts the target triple information from ELF header is changed for Mips platform. This patch is followed with a necessary change in clang driver.

echristo edited edge metadata.Feb 19 2015, 11:04 AM

Let's split this patch into two pieces to make the review easier - part of this patch is ok (#1) and I'd like to chat about #2 :)

  1. Passing MCOptions to the backend,
  2. Using it in the mips backend

Thanks!

-eric

lib/Target/Mips/MCTargetDesc/MipsAsmBackend.h
33 ↗(On Diff #20271)

You could probably store just the ABI here and unify the previous 3 class variables with just the triple rather than OSType.

dsanders added inline comments.Feb 23 2015, 9:22 AM
lib/Target/Mips/MCTargetDesc/MipsAsmBackend.h
33 ↗(On Diff #20271)

I don't think we should unify those variables. The triple is meaningless except in so far as it implies some default values.

For example, mips-linux-gnu in conjunction with the -mips64r2 option is equivalent to mips64-linux-gnu. Conversely mips64-linux-gnu and -mips32r2 is equivalent to mips-linux-gnu. I think it's better to keep the values separate and stick to initializing them from the options and (when lacking explicit options) the triple.

IsLittle can probably be derived from the triple but only because we have some dubious code to change the triple given to the backend when -EL and -EB are given to clang.

vmedic updated this revision to Diff 20577.Feb 24 2015, 1:49 AM
vmedic edited edge metadata.

The previous patch is split into two parts. This one contains changes to all targets and common code. Mips specific part will be moved to a separate patch.

dsanders resigned from this revision.Jul 12 2019, 4:10 PM