[PATCH] D13193: Add an MCTargetMachine and use it to configure MCAsmInfo

[Daniel Sanders via llvm-commits]

> -----Original Message-----
> From: Eric Christopher [mailto:echristo at gmail.com]
> Sent: 09 October 2015 02:32
> To: Daniel Sanders; grosbach at apple.com
> Cc: hfinkel at anl.gov; jholewinski at nvidia.com; jfb at chromium.org;
> Matthew.Arsenault at amd.com; dschuff at google.com;
> jyknight at google.com; llvm-commits at lists.llvm.org; renato.golin at linaro.org
> Subject: Re: [PATCH] D13193: Add an MCTargetMachine and use it to
> configure MCAsmInfo
> 
> echristo added a reviewer: grosbach.
> echristo added a comment.
> 
> 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 looks similar because it's the only sensible way to reach the destination, this is merely B on the journey from A to G. At this point it's mostly a wrapper (we have a small number of differences) but by the time we reach G, important parts will be reworked from a triple-focused form into a more target focused form that we can set according to the desired target.

See below for more detail as to why this is the right route to get to the destination.

> 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 information in those classes and their subclasses is seeded by the Triple. For Mips, this triple seeds these classes with insufficient and incorrect information which causes them to be configured incorrectly. I need to reach the point where MCTargetMachine is fully propagated and adapted to solve the problems of incorrect/insufficient information to resolve this misconfiguration. To re-use the A->G journey analogy, we need to be somewhere near E for these classes to be correctly configured. 

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

I completely agree with you, the hard part is getting there. Some targets have some unexpected dependencies between things that are supposed to be independent and some of these are mutual dependencies. MCSubtargetInfo is particularly tricky because of mutual dependencies in individual targets. I've found that if I'm not extremely careful with the order and method I introduce MCTargetMachine then the patch balloons to ridiculous proportions and ends up changing everything simultaneously. If I fold the ideal MCTargetMachine implementation into this patch as well then we end up with a huge and complex patch that's difficult to review. I want to make the journey incrementally by writing some big but trivial patches, followed by trickier but small and focused patches.

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

MCTargetOptions::ABIName is an interesting case. Several in-tree users don't bother to initialize MCTargetOptions and a few can't initialize it due to lack of information. The interesting bit is that you can only initialize it in one object per binary. Attempting to call InitMCTargetOptionsFromFlags in a second object requires you to include MCTargetOptionsCommandFlags.h which introduces duplicate definitions of the command line arguments and causes the program to abort. As a result, it has to be initialized outside the LLVM library and passed in. Unfortunately, this isn't possible in the case of the existing C-API since we can't change the interface. The C-API can either leave ABIName as "" in which case an LLVM that depends on it will misbehave (the Triple can't tell us what it should have been), or call InitMCTargetOptionsFromFlags() and cause callers of createMCAsmParser() (who must also call InitMCTargetOptionsFromFlags() to set the ABIName) to abort because of the duplicate command line arguments. There are other situations where multiple objects need to call InitMCTargetOptionsFromFlags() but they aren't substantially different to this example.

I've concluded that the right thing to do is to make MCTargetMachine the canonical home for the ABI name and (in later patches) provide a means to set it from a provided MCTargetOptions. Later on, we could move the canonical home back to MCTargetOptions but doing so doesn't serve any purpose.

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

Yes. I don't think it's the direction we disagreed on, just the stops on the way.

> 
> -eric
> 
> 
> http://reviews.llvm.org/D13193
> 
>