This is an archive of the discontinued LLVM Phabricator instance.

Add an MCTargetMachine and use it to configure MCAsmInfo
AbandonedPublic

Authored by dsanders on Sep 26 2015, 7:13 AM.

Details

Reviewers
grosbach
Summary

All targets require a registered MCTargetMachine, even if it's just
MCTargetMachine itself.

A small number of MCTargetMachine subclasses (namely PPCMCTargetMachine,
and X86MCTargetMachine) store the Triple in order to compare MacOSX
versions. Ideally, this would be possible without having to store a Triple.

A corresponding patch to clang will be committed along with this. The patch
is similar to the patches in the tools in ./tools/....

Mips currently obtains the ABI from the Triple architecture. This is wrong
since the architecture is the wrong source for this information, the value
is often misleading, and selecting the N32 ABI isn't possible. However,
it preserves the existing behaviour for the moment. A follow-up patch will
address these problems.

Follow up patches will expand the MCTargetMachine to other parts of the MC
layer.

Diff Detail

Event Timeline

dsanders updated this revision to Diff 35801.Sep 26 2015, 7:13 AM
dsanders retitled this revision from to Add an MCTargetMachine and use it to configure MCAsmInfo.
dsanders updated this object.
dsanders added subscribers: echristo, rengolin, llvm-commits.

As part of this change, you're creating classes whose primary function seems to be to cache results from calling member functions of Triple? Is there some performance benefit to doing so? Maintaining this pattern increases the maintenance burden on using Triple queries; why not just expose the Triple member for querying?

It's really about disconnecting an ambiguous representation of the target (implied by the triple) from the need for an unambiguous representation of the desired target.

The Triple is a representation of the given string and provides some methods to help understand the usual meaning of that string. However, as discussed in 'The Trouble with Triples' thread there are some problems with using the triple as the representation of the target.

The first problem is that the same string can have multiple effective meanings, either as a result of a particular build being configured to use meanings that are not the usual meaning (such as the default CPU/ABI or perhaps specific details being overridden), or as a result of command line processing overriding the default meaning. For this problem we would like our target description to definitively inform the backend how it should behave.

The second is that multiple strings can have the same meaning, either for one of the same reasons, or because they really are synonyms. For this problem we would like to resolve the multiple representations to a single description of the desired behaviour.

MCTargetMachine is trying solve both of these problems. It currently looks like it's only caching values since nothing has begun using the ability to mutate the MCTargetMachine before it's used for the first time. Over the next few weeks, I intend to post patches that will make use of the temporary mutability of MCTargetMachine to resolve various issues with the Mips target particularly in clang-based toolchains that are intended to be drop-in replacements for gcc.

Traveling this week so responses are delayed.

Traveling this week so responses are delayed.

No worries, thanks for letting me know.

dsanders updated this revision to Diff 36463.Oct 4 2015, 3:12 AM

Fix a mistake in the arguments to createMCTargetMachine(). The Target should
not be an argument to the outermost function.

Having written a couple follow-on patches and reached the point where MCSubtargetInfo can (almost) be migrated and used appropriately, there's an issue I need to bring up.

To avoid dereferencing null MCTargetMachine pointers held by partially initialized TargetMachines there's a need to pass a MCTargetMachine to createTargetMachine(). However, this makes it necessary to call createMCTargetMachine() from LLVMCreateTargetMachine(). We can't change the C-API so we have four options:

  1. Allow a memory leak. Obviously this isn't a good choice.
  2. Change TargetMachine to an is-a relationship with MCTargetMachine. This doesn't look possible because you end up with MipsTargetMachine is-a TargetMachine is-a MipsMCTargetMachine is-a MCTargetMachine and this can't happen for more than one target.
  3. Have Target/TargetMachine own MCTargetMachines allocated by LLVMCreateTargetMachine()
  4. Wrap TargetMachine pointers returned by the C-API in a new class that also owns the MCTargetMachine pointer for the existing C-API and have LLVMDisposeTargetMachine() delete the MCTargetMachine if it owns it.

I'm not keen on any of these but the fourth sounds most sensible since it keeps this problem in the C-API's domain without affecting C++. Thoughts?

Almost there :)

Back from vacation, digging out from under email.

Hi Daniel,

It looks like what you've done is take a wrapper around Triple and propagate it - basically redoing some of the TargetTuple work and just calling it MCTargetMachine. :)

It wasn't quite where I was going though, let me try and explain by taking a look at some of the normal TargetMachine class contents:

/// Low level target information such as relocation model. Non-const to
/// allow resetting optimization level per-function.
MCCodeGenInfo *CodeGenInfo;

/// Contains target specific asm information.
const MCAsmInfo *AsmInfo;

const MCRegisterInfo *MRI;
const MCInstrInfo *MII;
const MCSubtargetInfo *STI;

Where we've actually got a lot of the information you're passing your wrapped Triple into. The idea that I was trying to get across is that the MCTargetMachine should serve as the base holder class for a lot of this information and would be initialized similarly to the various TargetMachines, but with MC level equivalents where it matters, e.g. MCTargetOptions, etc.

If you look at MCTargetOptions, for example, it even has this:

/// getABIName - If this returns a non-empty string this represents the                      
/// textual name of the ABI that we want the backend to use, e.g. o32, or                    
/// aapcs-linux.                                                                             
StringRef getABIName() const;
std::string ABIName;

which will serve as a good way of solving your "I don't have the ABI" problem that you were mentioning before - as long as argument parsing will set it correctly when constructing MCTargetMachine.

Does this make sense? Seem like it's going the right direction to you?

-eric

I think I've caught on to the difference between your mental model and mine. It's just been tricky to notice it because the things you've said here fit perfectly into my mental model. :-)

In my mental model, I have an object named MCTargetMachine describing the desired target. I configure this however I want (starting with the triple then mutating it according to command line options). I then give this MCTargetMachine to MCCodeGenInfo, MCAsmInfo, MCRegisterInfo, MCInstrInfo, MCSubtargetInfo, etc. when constructing them and they consult it whenever they want to know certain properties of the target. When you spoke about MCTargetMachine holding information from these classes, I read it as moving the decision-making values into MCTargetMachine and having them ask MCTargetMachine for it which is what I was already doing.

I suspect your mental model is that MCTargetMachine holds an owning pointer to MCCodeGenInfo, MCAsmInfo, MCRegisterInfo, MCInstrInfo, MCSubtargetInfo, etc. and is also responsible for constructing them. This model works pretty much the same way as mine to begin with. You construct a MCTargetMachine and configure it. The difference is that the users then ask MCTargetMachine to create the various objects on their behalf and MCTargetMachine passes itself into their respective constructors in order to configure them. Is that what you're going for?

If that understanding is correct then there are two major steps here.

  • Encapsulate MC layer objects in MCTargetMachine and make it their owner. This is effectively moving user calls to createMC*() into MCTargetMachine.
  • Update the constructors to take an MCTargetMachine or at least information originating from it.

With my existing plan being the latter of these two steps. As far as the end result is concerned, I don't think it matters too much which order we do the steps.
I suppose we have less user-visible churn if we do the encapsulation of the MC layer objects first though.

You've got my mental model fairly well established. I'll take a look at
this yesterday and make sure it's going to work the way I want.

Thanks for your patience!

-eric

Great! I think it's best to abandon this patch and then I'll post a patch that just does the encapsulation step. After that, I'll post another version of this patch starting from that point. Does that sound good?

... is that MCTargetMachine holds an owning pointer to MCCodeGenInfo, MCAsmInfo ...

I've realised I made a mistake here. The pointers ought to be owned by the caller of MCTargetMachine::create*(). This is consistent with TargetMachine.

Great! I think it's best to abandon this patch and then I'll post a patch that just does the
encapsulation step. After that, I'll post another version of this patch starting from that point. Does that sound good?

Do note that what you're talking about as "the TargetMachine is passed down" only happens for
very few objects underneath TargetMachine and that generally things are constructed with the bare
minimum of information.

I'm finding that some of these use more information than they first appear to but I agree that we should generally be passing information down rather than the whole object.

... is that MCTargetMachine holds an owning pointer to MCCodeGenInfo, MCAsmInfo ...

I've realised I made a mistake here. The pointers ought to be owned by the caller of MCTargetMachine::create*(). This is consistent with TargetMachine.

I don't agree with this part. I think the ownership hierarchy should be similar to TargetMachine in the way that TargetMachine owns TargetSubtargetInfo and it owns the things that depend upon it.

Sorry, I've been a bit unclear here. I mean that the create*() functions themselves give ownership to their caller. TargetMachine holds some ownership resulting from LLVMTargetMachine's calls to TargetMachine's create*() functions.

By the way, some of those members (e.g. MRI) look like they should be unique_ptr's. TargetMachine is manually deleting them in its destructor but nothing seems to set them aside from LLVMTargetMachine's calls to create*() in LLVMTargetMachine::initAsmInfo(). Do you agree?


From: Eric Christopher [echristo@gmail.com]
Sent: 17 October 2015 12:17
To: reviews+D13193+public+f5e72ebae342dcc9@reviews.llvm.org; Daniel Sanders; grosbach@apple.com
Cc: hfinkel@anl.gov; jholewinski@nvidia.com; jfb@chromium.org; Matthew.Arsenault@amd.com; dschuff@google.com; jyknight@google.com; llvm-commits@lists.llvm.org; renato.golin@linaro.org
Subject: Re: [PATCH] D13193: Add an MCTargetMachine and use it to configure MCAsmInfo

dsanders abandoned this revision.Oct 19 2015, 2:23 AM

Abandoning in favour of a different patch series. I'll post the new one shortly.