Page MenuHomePhabricator

Refactor `initTargetOptions` out of `EmitAssemblyHelper::CreateTargetMachine` and use it to initialize TargetOptions for ThinLTO Backends
AbandonedPublic

Authored by tejohnson on Mar 17 2017, 11:12 PM.

Details

Reviewers
pcc
mehdi_amini

Event Timeline

mehdi_amini created this revision.Mar 17 2017, 11:12 PM
tejohnson edited edge metadata.Mar 18 2017, 8:28 AM

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.

clang/lib/CodeGen/BackendUtil.cpp
595

What about this - it isn't used in the TargetOptions. There are a few other things passed directly to createTargetMachine below. Will -fpic get handled correctly, e.g.? Ah, looks like lto::Config has fields for that and the code model etc, that are essentially getting the default values on this path (when used via clang - that will need to be set up in lto::Config in runThinLTOBackend as well.

982

Why did this move?

mehdi_amini added inline comments.Mar 18 2017, 10:26 AM
clang/lib/CodeGen/BackendUtil.cpp
982

"History": it is useless now. I first refactored the initTargetOptions as a member on EmitAssemblyHelper rather than a static function.

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 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 D31100 and D31101 are the right approach?

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.

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

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

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

Ping - I think this just needs a test in addition to undoing the AsmHelper definition move. I have some follow on changes I want to send that set up the rest of the lto::Config fields (e.g. the RelocModel etc).

As discussed with Mehdi offline, I am taking this one over. I just mailed D31508 which supersedes this one.

tejohnson commandeered this revision.Mar 30 2017, 1:44 PM
tejohnson abandoned this revision.
tejohnson edited reviewers, added: mehdi_amini; removed: tejohnson.