tejohnson added a comment.

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

> 
> 
>> Ah, I see that in thinBackend the created TM is used by 
>> handleAsmUndefinedRefs and opt(), so this patch is complementary. But in 
>> that case, to set up the TM accurately, this code does need to set up the 
>> other fields of lto::Config read by createTargetMachine, e.g. RelocModel, 
>> CodeModel, etc.
> 
> Right. 
>  Forcing to use the LTO API for this ensures completeness: there isn't any 
> target flag that clang could use that wouldn't be exposed through LTO.

I think that is already the case, here we aren't changing what is exposed 
through LTO just how we invoke it from this path.


https://reviews.llvm.org/D31114



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to