This is an archive of the discontinued LLVM Phabricator instance.

Replace Triple with a new TargetTuple in MCTargetDesc/* and related. NFC.
ClosedPublic

Authored by dsanders on Jul 6 2015, 11:40 AM.

Details

Summary

This is the first patch in the series to migrate Triple's (which are ambiguous)
to TargetTuple's (which aren't).

For the moment, TargetTuple simply passes all requests to the Triple object it
holds. Once it has replaced Triple, it will start to implement the interface in
a more suitable way.

This change makes some changes to the public C++ API. In particular,
InitMCSubtargetInfo(), createMCRelocationInfo(), and createMCSymbolizer()
now take TargetTuples instead of Triples. The other public C++ API's have
been left as-is for the moment to reduce patch size.
Clang and other in-tree tools will be updated with a trivial patch when
this is committed.

Diff Detail

Event Timeline

dsanders updated this revision to Diff 29111.Jul 6 2015, 11:40 AM
dsanders retitled this revision from to Replace Triple with a new TargetTuple in MCTargetDesc/* and related. NFC..
dsanders updated this object.
dsanders added subscribers: rengolin, llvm-commits.
emaste added a subscriber: emaste.Jul 6 2015, 6:41 PM

Hi Daniel,

I have only looked at the header and implementation of TargetTuple, and I have a few comments on comments. :)

I'll continue looking at the other changes in due time.

cheers,
--renato

include/llvm/ADT/TargetTuple.h
39

I absolutely agree!

65

This has to go soon. Please add a FIXME to make sure we merge this into arm/armeb.

87

These enums will become authoritative once Triple is gone.

Even if we don't remove Triple forever, it must use these, and not its own, anyway.

210

I don't think we should encourage people re-writing the constructor, or it would be a nightmare to investigate issues.

One could use the default constructor and set ABI to 32-bits later, which would be a simpler patch to keep for both vendor and community.

284

why two?

lib/Support/TargetTuple.cpp
16
// FIXME: These should go as soon as Triple is dead

Just to be sure people don't come here and say "OMG! WTF?".

Phew! Ok, looks ok to me, with the comments.

About the Tuple+CPU+Features (and others), unless you're planning to do that move in this streak, I think it'd be best to keep a small FIXME comment to use CPU/Features/Options in the Tuple itself.

include/llvm/Support/TargetRegistry.h
119

I think this one deserves a FIXME. CPU should be already uniquely identifiable in the tuple.

351

Same here. I'd expect (in the future) something like this:

TargetTuple Tuple (Triple(TT)); // Triple for now, TT directly later
Tuple.setCPU(CPU);
Tuple.setFeatures(Features);
return MCSubtargetInfoCtorFn(Tuple);
dsanders added inline comments.Jul 7 2015, 7:14 AM
include/llvm/ADT/TargetTuple.h
87

Ok, I'll drop the FIXME for these but keep the one for ArchType for now.

210

This is trying to account for configure-option ambiguity in the GNU triples. For GCC, it's fairly common for distributors to change the meanings of triples using options like --with-arch, --with-cpu, --with-abi, etc. There are some target-specific options such as --with-mode to select ARM/Thumb by default and --with-nan for MIPS NAN encoding selection. I'm not sure about other targets but the MIPS options are in very common use and is likely to get more common in the near future as we try to push towards IEEE754(2008) being the conventional floating point standard on our target (we're still on 1985 for the majority of software).

The thinking behind this comment is that that distributors need to do this kind of thing to make LLVM's GNU triples consistent with the rest of their distribution. We can therefore save ourselves a fair bit of pain by nominating the right place to do it. The advantage, is that LLVM (and related tools) can be made to have the same Triple meaning as the rest of the distribution. The downside is that, as you say, it makes investigating issues more difficult on distributions that do change it since they'll have a different default TargetTuple for the same Triple. TargetTuples are still unique and authoratative so we can compensate for this by using the tuple to investigate issues, effectively bypassing any customizations the distributor made in this constructor. Frontends may need to add command line options to allow TargetTuple's (or we could claim an unused subset of the triple strings) but that should be easy. We don't need to do the same thing for backends/LTO since we'll need the TargetTuple to be serialized in the LLVM-IR to avoid information loss anyway.

284

At the moment, its just to preserve Triple's interface which had both. I've been rewriting instances of .getTriple() to .str() as I notice them. At some point, I'd like to rewrite the remainder and delete getTriple().

Triple::getTriple() is fairly common, but it's also the least clear of the two. The name sounds like it ought to return 'this' :-)

lib/Support/TargetTuple.cpp
16

Sure, I've paraphrased a bit in the update (which I'll post shortly) since it's really about replacing the Triple member (GnuTT).

About the Tuple+CPU+Features (and others), unless you're planning to do that move in this streak, I think it'd be best to keep a small FIXME comment to use CPU/Features/Options in the Tuple itself.

I'm not planning to merge CPU name and Features into the TargetTuple in this series, but I probably should do this at some point. I'll add some FIXME's for now

include/llvm/Support/TargetRegistry.h
119

CPU should be already uniquely identifiable in the tuple.

I don't think that's true at the moment since Triples/TargetTuples lack the variety of values that the CPU name has. For example, -mcpu=pentium would cause CPU to be 'pentium' but the Triple/TargetTuple doesn't know about this string.

Are you thinking of the SubArch field that ARM and Kalimba use?

Wait, can you give a rundown of what your TargetTuple is doing etc? Maybe a
mail to the list? This seems weird.

Thanks!

-eric

Wait, can you give a rundown of what your TargetTuple is doing etc? Maybe a
mail to the list? This seems weird.

Hi Eric,

This is an old project (5+ years) that is finally realising, and had many threads in the list (maybe not with "TargetTuple" in it). Daniel is working towards that goal for some good weeks now, but if you feel we need another round, I think we better have it. I want everyone to be on the same boat as we thread these dangerous waters.

Maybe Daniel would be a better person to write up a summary of the changes so far and the plan for the future, as I was more focused on the TargetParser changes.

cheers,
--renato

include/llvm/ADT/TargetTuple.h
210

Fair enough.

284

Ok.

Ok, I'll post a write up of the problem(s) with GNU Triples and the planned solution in the morning.

Excellent. Thanks Daniel!

-eric

dsanders updated this revision to Diff 29654.Jul 14 2015, 2:36 AM
dsanders marked 6 inline comments as done.

Added/removed FIXME comments

@echristo: Have you had chance to read the "The Trouble with Triples" thread? Do you still have concerns?

include/llvm/ADT/TargetTuple.h
210

I've left the paragraph about patching the constructor in for the moment but I've added a fixme indicating that it will be replaced by CMake/config-files asap. Is that ok for this patch?

dsanders updated this revision to Diff 30152.Jul 20 2015, 5:53 AM

Refreshed patch and ping.
Removed paragraph about patching the TargetTuple constructor.

@echristo: Did the 'The Trouble with Triples' thread resolve everything for you?
I'm a bit stuck at the moment since without further comments I have no obvious
course of action to make progress with this patch series.

Ping

It's been three weeks since I posted the explanation on LLVMdev and Eric hasn't said whether the thread satisfied him or not. I'm not sure what to do at this point since this patch appears to be blocked on the "Could you explain? This seems weird" comment but I don't seem to have any means of unblocking it beyond what I've already done.
The patch series queued behind this is difficult to maintain so I'd greatly appreciate advice on how I can unblock this.

Have you tried pinging Eric on IRC? That usually helps a lot.

Not yet. I haven't used IRC for a very long time (>10 years) but I can get it set up and try that.

I'm here, just been very busy. I'll get to it today.

-eric

Thanks.


From: Eric Christopher [echristo@gmail.com]
Sent: 28 July 2015 16:37
To: Daniel Sanders; reviews+D10969+public+5f3f4828b5695b54@reviews.llvm.org
Cc: danalbert@google.com; srhines@google.com; javed.absar@arm.com; emaste@freebsd.org; jholewinski@nvidia.com; tberghammer@google.com; ted.woodward@codeaurora.org; jfb@chromium.org; llvm-commits@cs.uiuc.edu
Subject: Re: [PATCH] D10969: Replace Triple with a new TargetTuple in MCTargetDesc/* and related. NFC.

I'm here, just been very busy. I'll get to it today.

-eric

dsanders updated this revision to Diff 32960.Aug 24 2015, 8:37 AM

Refresh patch

dsanders updated this revision to Diff 34223.Sep 8 2015, 9:14 AM

Refresh and ping.

This project has stalled for a while so to try to get things moving again: The llvmdev thread goes into more detail but the major goals of the project this patch is part of include:

  • Fix the incorrect assumption that GNU triples describe a single specific target.
  • Allow vendors/distributions to configure the triple meanings to suit their target. Different vendors/distributions often have conflicting meanings for the same triple and the meanings change over time
  • Minimize the amount of LLVM that needs to be aware of GNU triple ambiguity (same triple but different meaning).
  • Minimize the amount of LLVM that needs to be aware of GNU triple aliases (different triple but same meaning).
  • Minimize the occasions where the GNU triple is parsed.

ARM and Mips are the main beneficiaries of this since our respective toolchain histories have made our GNU triples a big mess of ambiguity and inconsistency. However, other architectures (including X86) also share these problems to varying degrees.

The high level plan that was discussed in the 'The Trouble With Triples' thread was:

  1. Replace any remaining std::string's and StringRef's containing GNU triples with Triple objects.
  2. Split the llvm::Triple class into llvm::Triple and llvm::TargetTuple classes. Both are identical in implementation and almost identical in interface at this stage.
  3. Gradually replace Triples with TargetTuples until the C APIs and the LLVM-IR are the only place inside LLVM where Triples are still used.
  4. Change the implementation of TargetTuple to whatever is convenient for LLVM's internals and decide on a serialization.
  5. Replace serialized Triples with serialized TargetTuples in LLVM-IR. a. Maintain backwards compatibility with IR using triples, at least for a while.
  6. Add TargetTuple support to the C API. Exact API is t.b.d.
  7. Have the API users mutate the TargetTuple appropriately.

Step 5 was a little controversial on that thread, but it's not a significant change to the plan to keep the GNU triple around for LLVM-IR's purposes.

This patch is step 2 and the first piece of step 3. My current branch has ~90% of step 3 complete and can be found at https://github.com/dsandersimgtec/llvm/tree/triple-trouble if you'd like to peek at some of the patches that will follow this one.

rengolin accepted this revision.Sep 9 2015, 9:31 AM
rengolin added a reviewer: rengolin.

Hi Daniel,

For what it is, the patch looks good to me.

It's a big mechanical patch that implements a Tuple wrapper to Triple and change all places that used to use Triples. I don't expect it to clean up anything, since for now we're just moving things alone, but would be good to make Triple use the Tuple enums before deprecating it, so that you can remove a lot of code inserted by this patch.

If people feel uncomfortable with so much being added spuriously, maybe you could have a go at doing that change now. But I don't think it's strictly necessary and will leave at your discretion. We need to move this thing forward as it has taken too long and too much of the ARM and MIPS architectural support depends on this.

Not that this could change anything anywhere else, but It'd be good to have other back-end owners to agree, since this is a big change. I believe most people are copied in this review, anyway, and had time enough to care.

This revision is now accepted and ready to land.Sep 9 2015, 9:31 AM

I'm sorry for all the delays, I'm trying to get back to this and haven't forgotten it (or the other thread). My apologies.

-eric

Thanks Renato.

I don't expect it to clean up anything, since for now we're just moving things alone, but would be good to make Triple use the Tuple enums before deprecating it, so that you can remove a lot of code inserted by this patch.

We'd want to bring back separate enums eventually (see below) but if deferring that piece helps get the overall patch series moving then I don't mind doing that.

The reason for separating the enums is that it's desirable for Triple and TargetTuple to have different sets of values, particularly for ARM and Mips. By the end of the project, Triple's values should be focused on representing the triple, and TargetTuple's values should be focused on representing the target. For example, it's correct for mipsel-linux-gnu that Triple::getArch() returns Triple::mipsel. However, it's also desirable that TargetTuple::getArch() return TargetTuple::mips and TargetTuple::getEndian() return TargetTuple::LittleEndian. To put it another way, Triple's enum are more-or-less numeric representations of the input string, whereas TargetTuple's enums are representations of the intended target.

I don't know the ARM triples but here's an example based on what I've seen in your target parser work: Triple::getArch() should return Triple::armv7 for armv7-linux-gnu, while TargetTuple::getArch() would return TargetTuple::arm, and TargetTuple::getSubArch() would return TargetTuple::v7. Does that agree with your understanding of ARM triples?

From Eric's comment:

I'm sorry for all the delays, I'm trying to get back to this and haven't forgotten it (or the other thread). My apologies.

I don't want to pressure you as I know you're busy but could you be a little more specific than that? My biggest worry is that the issues this project resolves are blocking our toolchain products and while the schedule for some of those feels like a long time, I'm also concious of the fact that sorting these issues out is a multi-month project.

The reason for separating the enums is that it's desirable for Triple and TargetTuple to have different sets of values, particularly for ARM and Mips. By the end of the project, Triple's values should be focused on representing the triple, and TargetTuple's values should be focused on representing the target. For example, it's correct for mipsel-linux-gnu that Triple::getArch() returns Triple::mipsel. However, it's also desirable that TargetTuple::getArch() return TargetTuple::mips and TargetTuple::getEndian() return TargetTuple::LittleEndian. To put it another way, Triple's enum are more-or-less numeric representations of the input string, whereas TargetTuple's enums are representations of the intended target.

You're absolutely right, and that's why I ended up creating my own enums in the TargetParser.

I don't know the ARM triples but here's an example based on what I've seen in your target parser work: Triple::getArch() should return Triple::armv7 for armv7-linux-gnu, while TargetTuple::getArch() would return TargetTuple::arm, and TargetTuple::getSubArch() would return TargetTuple::v7. Does that agree with your understanding of ARM triples?

Yes. Moreover, the sub-arch should be an identifier for a set of default features + allowed features, that can be enabled/disabled via other flags. The fact that the mapping between CPU and Arch is not injectve nor surjective makes the meaning of arch and subarch very subjective. :)

This revision was automatically updated to reflect the committed changes.

(apologies if this double-posts, it seems Phabricator just died)

I've committed this (along with the trivial clang patch) in r247683 on the grounds that I have Renato's LGTM and I don't have any pending review comments. It doesn't seem reasonable to continue to wait for Eric with no indication of when he might be able to take a look at it. For the record, I've been waiting since July and my request last week for an indication as to when he might find time has not been replied to. Of course, post-commit review comments are still welcome and I'm happy to follow up on them.

I started a reply yesterday actually and I'm sorry I didn't finish it.
Briefly I don't think this is the right direction and I don't think you've
explained why this is the right direction. The serialization that you have
planned is insufficient and doesn't take into account portability, I don't
like the idea of using global metadata for it and you haven't explained why
this is acceptable. I actually was spending a non-trivial amount of time
trying to design a way for this to work along side the assembler and main
compiler (which I don't think you've bothered to try to do)

Please revert this immediately.

-eric

I started a reply yesterday actually and I'm sorry I didn't finish it.

Briefly I don't think this is the right direction and I don't think you've explained why this is the right direction.

The serialization that you have planned is insufficient and doesn't take into account portability

I don't like the idea of using global metadata for it and you haven't explained why this is acceptable


From: Eric Christopher [echristo@gmail.com]
Sent: 15 September 2015 16:56
To: Daniel Sanders; reviews+D10969+public+5f3f4828b5695b54@reviews.llvm.org; renato.golin@linaro.org
Cc: jyknight@google.com; dschuff@google.com; Matthew.Arsenault@amd.com; stanislav.mekhanoshin@amd.com; danalbert@google.com; srhines@google.com; javed.absar@arm.com; emaste@freebsd.org; jholewinski@nvidia.com; tberghammer@google.com; ted.woodward@codeaurora.org; jfb@chromium.org; llvm-commits@lists.llvm.org
Subject: Re: [PATCH] D10969: Replace Triple with a new TargetTuple in MCTargetDesc/* and related. NFC.

I started a reply yesterday actually and I'm sorry I didn't finish it. Briefly I don't think this is the right direction and I don't think you've explained why this is the right direction. The serialization that you have planned is insufficient and doesn't take into account portability, I don't like the idea of using global metadata for it and you haven't explained why this is acceptable. I actually was spending a non-trivial amount of time trying to design a way for this to work along side the assembler and main compiler (which I don't think you've bothered to try to do)

Please revert this immediately.

-eric

Sorry, I accidentally hit Ctrl+Enter. My next email will contain a reply :-)


From: Daniel Sanders
Sent: 15 September 2015 17:05
To: Eric Christopher; reviews+D10969+public+5f3f4828b5695b54@reviews.llvm.org; renato.golin@linaro.org
Cc: jyknight@google.com; dschuff@google.com; Matthew.Arsenault@amd.com; stanislav.mekhanoshin@amd.com; danalbert@google.com; srhines@google.com; javed.absar@arm.com; emaste@freebsd.org; jholewinski@nvidia.com; tberghammer@google.com; ted.woodward@codeaurora.org; jfb@chromium.org; llvm-commits@lists.llvm.org
Subject: RE: [PATCH] D10969: Replace Triple with a new TargetTuple in MCTargetDesc/* and related. NFC.

I started a reply yesterday actually and I'm sorry I didn't finish it.

Briefly I don't think this is the right direction and I don't think you've explained why this is the right direction.

The serialization that you have planned is insufficient and doesn't take into account portability

I don't like the idea of using global metadata for it and you haven't explained why this is acceptable


From: Eric Christopher [echristo@gmail.com]
Sent: 15 September 2015 16:56
To: Daniel Sanders; reviews+D10969+public+5f3f4828b5695b54@reviews.llvm.org; renato.golin@linaro.org
Cc: jyknight@google.com; dschuff@google.com; Matthew.Arsenault@amd.com; stanislav.mekhanoshin@amd.com; danalbert@google.com; srhines@google.com; javed.absar@arm.com; emaste@freebsd.org; jholewinski@nvidia.com; tberghammer@google.com; ted.woodward@codeaurora.org; jfb@chromium.org; llvm-commits@lists.llvm.org
Subject: Re: [PATCH] D10969: Replace Triple with a new TargetTuple in MCTargetDesc/* and related. NFC.

I started a reply yesterday actually and I'm sorry I didn't finish it. Briefly I don't think this is the right direction and I don't think you've explained why this is the right direction. The serialization that you have planned is insufficient and doesn't take into account portability, I don't like the idea of using global metadata for it and you haven't explained why this is acceptable. I actually was spending a non-trivial amount of time trying to design a way for this to work along side the assembler and main compiler (which I don't think you've bothered to try to do)

Please revert this immediately.

-eric

I've replied in a different thread with what I see is the design that needs
to happen here.

-eric

BTW I do agree that it's completely unreasonable how long it's taken me on
this. I do truly apologize, it was only when looking at the recent patch
dealing with ABIInfo that you approved that I realized what you're trying
to deal with in this and how it's interacting with everything else. It's
absolutely needed so let's discuss and move this forward.

-eric

Thanks for the reply. We should hold the main conversation in the llvmdev thread but to briefly reply here.

Briefly I don't think this is the right direction and I don't think you've explained why this is the right direction.

Do you at least accept that Triples are not usable as a way to describe the target? and that they are ambiguous and inconsistent in their meanings?

The serialization that you have planned is insufficient and doesn't take into account portability

The exact serialization doesn't matter too much so long as there's room to grow it. Both my original string representation and the metadata approach that was later proposed have this capability.

I don't like the idea of using global metadata for it and you haven't explained why this is acceptable

FWIW, my original proposal was a 'target tuple' statement to match the 'target triple' one we have today. Whatever serialization approach we choose doesn't significantly change the overall direction and has no effect on the current phase of the project. There's no need to block the early phases of the project on small details of the later phases.

I actually was spending a non-trivial amount of time trying to design a way for this to work along side the assembler and main compiler (which I don't think you've bothered to try to do)

What makes you say I haven't bothered to design a way to work with the assembler and the main compiler? That's one of the core concepts driving the work and working compatibly with other toolchains is the objective.


From: Eric Christopher [echristo@gmail.com]
Sent: 15 September 2015 16:56
To: Daniel Sanders; reviews+D10969+public+5f3f4828b5695b54@reviews.llvm.org; renato.golin@linaro.org
Cc: jyknight@google.com; dschuff@google.com; Matthew.Arsenault@amd.com; stanislav.mekhanoshin@amd.com; danalbert@google.com; srhines@google.com; javed.absar@arm.com; emaste@freebsd.org; jholewinski@nvidia.com; tberghammer@google.com; ted.woodward@codeaurora.org; jfb@chromium.org; llvm-commits@lists.llvm.org
Subject: Re: [PATCH] D10969: Replace Triple with a new TargetTuple in MCTargetDesc/* and related. NFC.

I started a reply yesterday actually and I'm sorry I didn't finish it. Briefly I don't think this is the right direction and I don't think you've explained why this is the right direction. The serialization that you have planned is insufficient and doesn't take into account portability, I don't like the idea of using global metadata for it and you haven't explained why this is acceptable. I actually was spending a non-trivial amount of time trying to design a way for this to work along side the assembler and main compiler (which I don't think you've bothered to try to do)

Please revert this immediately.

-eric

BTW I do agree that it's completely unreasonable how long it's taken me on this. I do truly apologize, ...

Thanks. I appreciate that.


From: Eric Christopher [echristo@gmail.com]
Sent: 15 September 2015 17:27
To: Daniel Sanders; reviews+D10969+public+5f3f4828b5695b54@reviews.llvm.org; renato.golin@linaro.org
Cc: jyknight@google.com; dschuff@google.com; Matthew.Arsenault@amd.com; stanislav.mekhanoshin@amd.com; danalbert@google.com; srhines@google.com; javed.absar@arm.com; emaste@freebsd.org; jholewinski@nvidia.com; tberghammer@google.com; ted.woodward@codeaurora.org; jfb@chromium.org; llvm-commits@lists.llvm.org
Subject: Re: [PATCH] D10969: Replace Triple with a new TargetTuple in MCTargetDesc/* and related. NFC.

BTW I do agree that it's completely unreasonable how long it's taken me on this. I do truly apologize, it was only when looking at the recent patch dealing with ABIInfo that you approved that I realized what you're trying to deal with in this and how it's interacting with everything else. It's absolutely needed so let's discuss and move this forward.

-eric

Briefly I don't think this is the right direction and I don't think you've explained why this is the right direction.

Do you at least accept that Triples are not usable as a way to describe the target? and that they are ambiguous and inconsistent in their meanings?

I think that they're insufficient to fully describe a target given command line options that can change defaults is a better way of putting it.

Ok, so we've got some agreement on this point. Most of the thinking follows on from here.

I would say that the Triples are insufficient as a starting point too but we can follow that thread on the llvm-dev thread.

I don't like the idea of using global metadata for it and you haven't explained why this is acceptable

FWIW, my original proposal was a 'target tuple' statement to match the 'target triple' one we have today.
Whatever serialization approach we choose doesn't significantly change the overall direction and has no
effect on the current phase of the project. There's no need to block the early phases of the project on small
details of the later phases.

Sure, as long as the general direction seems good. IR level serialization of things is ... touchy and I'd like to
make sure that we get something there that we're going to be happy with long term given the stability
guarantees. It's easier to define things on the side that can change rather than make a mistake at the IR level
in a way that's not easy to upgrade. (Which we'll also need for any triple change as well).

Fair enough. I was expecting the IR change to be the first major debate on the implementation detail. My current patch series stops just before it for this reason.

I actually was spending a non-trivial amount of time trying to design a way for this to work along side the assembler and main compiler (which I don't think you've bothered to try to do)

What makes you say I haven't bothered to design a way to work with the assembler and the main compiler?
That's one of the core concepts driving the work and working compatibly with other toolchains is the objective.

I think it's trying to solve a problem in the assembler interfaces, but from the wrong tack. I hope I explained it in one of my followups.

Yes, but not only that. It's trying to solve several big issues simultaneously since they have a common origin point even though the causes are different.


From: Eric Christopher [echristo@gmail.com]
Sent: 15 September 2015 17:35
To: Daniel Sanders; reviews+D10969+public+5f3f4828b5695b54@reviews.llvm.org; renato.golin@linaro.org
Cc: jyknight@google.com; dschuff@google.com; Matthew.Arsenault@amd.com; stanislav.mekhanoshin@amd.com; danalbert@google.com; srhines@google.com; javed.absar@arm.com; emaste@freebsd.org; jholewinski@nvidia.com; tberghammer@google.com; ted.woodward@codeaurora.org; jfb@chromium.org; llvm-commits@lists.llvm.org
Subject: Re: [PATCH] D10969: Replace Triple with a new TargetTuple in MCTargetDesc/* and related. NFC.