Page MenuHomePhabricator

The ELF container needs to depend on the ABI rather than the target triple.
AbandonedPublic

Authored by vmedic on Nov 3 2014, 8:12 AM.

Details

Reviewers
sstankovic
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.

Diff Detail

Event Timeline

vmedic updated this revision to Diff 15712.Nov 3 2014, 8:12 AM
vmedic retitled this revision from to 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.
dsanders edited edge metadata.Nov 6 2014, 7:10 AM
dsanders added a subscriber: rafael.

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.

lib/Target/Mips/MCTargetDesc/MipsABIInfo.cpp
35–46

This doesn't belong in MipsABIInfo, it's selecting between processors.

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

This behaves correctly but it feels a bit unintuitive. On my first read, I was expecting it to construct be UnknownABI but it actually builds the value from the -target-abi option (if given, otherwise UnknownABI).

Always subscribe llvm-commits, cfe-commits or lldb-commits! Please
create a new revision, adding one of the lists.

vmedic updated this revision to Diff 16374.Nov 19 2014, 4:05 AM
vmedic edited edge metadata.
vmedic added a subscriber: Unknown Object (MLST).

This is the patch that has been updated to the top of the tree with no functional changes. 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.

dsanders resigned from this revision.Dec 11 2014, 3:16 AM
dsanders removed a reviewer: dsanders.

This revision has been replaced by one that had llvm-commits CC'd from the start. Resigning from this revision to remove it from my phabricator queue (I'm still on the new revision)