[PATCH] D31114: Refactor `initTargetOptions` out of `EmitAssemblyHelper::CreateTargetMachine` and use it to initialize TargetOptions for ThinLTO Backends

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.

In https://reviews.llvm.org/D31114#704749, @mehdi_amini wrote:

> In https://reviews.llvm.org/D31114#704748, @tejohnson wrote:
>
> > In https://reviews.llvm.org/D31114#704733, @mehdi_amini wrote:
> >
> > > In https://reviews.llvm.org/D31114#704728, @tejohnson wrote:
> > >
> > > > In https://reviews.llvm.org/D31114#704726, @mehdi_amini wrote:
> > > >
> > > > > In https://reviews.llvm.org/D31114#704649, @tejohnson wrote:
> > > > >
> > > > > > I think you won't get the correct handling of -emit-llvm and -emit-llvm-bc since we don't get the handling for Backend_Emit* in EmitAssemblyHelper::EmitAssembly.
> > > > >
> > > > >
> > > > > I was not trying to achieve this. And your approach of an "OptimizeOnly" or "DisableCodeGen" on the lot::Config for this purpose makes sense.
> > > >
> > > >
> > > > I'm confused - are you saying you now think https://reviews.llvm.org/D31100 and https://reviews.llvm.org/D31101 are the right approach?
> > >
> > >
> > > I believe your patch is the right approach when clang needs to get the optimized IR, which is the case for -emit-llvm/-emit-bc. I believe that it shouldn't do that when it expects an object file.
> >
> >
> > What about for -S, which presumably maps to Backend_EmitAssembly?
>
>
> Should be handled by LTO (we added it recently I think, at least to libLTO).
>
> > The LTO Config does have a CodeGenFileType field, which defaults to ObjectFile, but could be set here to AssemblyFile (see the handling in EmitAssemblyHelper::AddEmitPasses). I believe if you include the test case I added in https://reviews.llvm.org/D31101 which emits assembly that it will fail with this patch, for example. Since clang already has to handle codegen for all output types, I am not sure why it is better to have the LTO API handle the output for some of the cases and not others?
>
> It is not better, and I don't suggest that. I believe all the cases should go through the LTO API preferably (or the LTO API should always use clang for codegen, but I don't believe anyone would consider that).


I went ahead and updated my clang patch https://reviews.llvm.org/D31101 to only fall back to the clang handling in the -emit-llvm and -emit-llvm-bc cases.

I removed the test changes I had made to test/CodeGen/function-sections.c, you can include them here instead as a test for this fix.


https://reviews.llvm.org/D31114