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

[Daniel Sanders via llvm-commits]

> > 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 at gmail.com]
Sent: 17 October 2015 12:17
To: reviews+D13193+public+f5e72ebae342dcc9 at reviews.llvm.org; 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



On Fri, Oct 16, 2015 at 7:23 AM Daniel Sanders <daniel.sanders at imgtec.com<mailto:daniel.sanders at imgtec.com>> wrote:
dsanders added a comment.

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.

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

-eric


http://reviews.llvm.org/D13193



-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151017/5e278632/attachment.html>