Page MenuHomePhabricator

Pass code-model through Module IR to LTO which will use is
ClosedPublic

Authored by cmtice on Sep 20 2018, 2:15 PM.

Details

Summary

Currently the code-model does not get saved in the module IR, so if a code model is specified when compiling with LTO, it gets lost and is not propagated properly to LTO. This patch (along with one to the Clang front end: https://reviews.llvm.org/D52323) fixes that.

Depends on https://reviews.llvm.org/D52323

Fixes PR33306.

Diff Detail

Repository
rL LLVM

Event Timeline

cmtice created this revision.Sep 20 2018, 2:15 PM
cmtice edited the summary of this revision. (Show Details)Sep 20 2018, 2:20 PM
dexonsmith added inline comments.Sep 20 2018, 3:32 PM
lib/IR/Module.cpp
522 ↗(On Diff #166358)

Is this the correct linking behaviour for code model? I.e., is it illegal to link two object files together that have different code models?

tmsriram added inline comments.
lib/IR/Module.cpp
522 ↗(On Diff #166358)

I believe it is undefined behavior to link two object files with different code models as the compiler has to generate additional code if a larger code model would be used. As an example:

extern int *a;
int main() {

return a[10];

}

$ clang a.cc -S -mcmodel=large && cat a.s
...
movabsq $a, %rax
movq (%rax), %rax
movl 40(%rax), %eax
popq %rbp
..

$ clang a.cc -S -mcmodel=small && cat a.s
...
movq a, %rax
movl 40(%rax), %eax
popq %rbp

The data would not be accessible with a 32-bit relocation for x86_64 and the compiler must explicitly handle this.

tejohnson added inline comments.Sep 21 2018, 8:23 AM
lib/IR/Module.cpp
522 ↗(On Diff #166358)

Presumably the largest could be picked for code generation, but if it is illegal to link 2 native objects with different code models then the question is why we should support linking to LTO IR objects with different code models. Also, not sure how something like -code-model=kernel interact with the -code-model=small/medium/large.

Unless otherwise shown that we need to mix and match different code models, I think Error is the right behavior. Suggest adding a comment summarizing why it is being treated as an Error.

test/LTO/X86/codemodel-3.ll
2 ↗(On Diff #166358)

The Inputs/codemodel-3.ll file is missing from patch.

dexonsmith added inline comments.Sep 21 2018, 8:32 AM
lib/IR/Module.cpp
522 ↗(On Diff #166358)

Agreed; if it's not correct for normal object files we should error here as in the patch.

cmtice updated this revision to Diff 166494.Sep 21 2018, 9:46 AM
cmtice edited the summary of this revision. (Show Details)

Add comment explaining why mixing two code models is an error. Add missing Inputs/codemodel-3.ll file.

tejohnson added inline comments.Sep 21 2018, 10:01 AM
lib/LTO/LTOBackend.cpp
143 ↗(On Diff #166494)

With this change the CodeModel passed in to the LTO link (if any) is ignored. I think this should be like the code that sets the RelocModel just above here.

(As an aside, please upload your patches with context. You can either do so automatically if you use the 'arc' tool, or I think there is a way to do when you create the diff directly - see https://llvm.org/docs/Phabricator.html)

cmtice updated this revision to Diff 166505.Sep 21 2018, 10:35 AM

Use Conf.CodeModel in LTO, if it exists.

tejohnson accepted this revision.Sep 21 2018, 10:42 AM

LGTM with a minor change suggested.

lib/LTO/LTOBackend.cpp
145 ↗(On Diff #166505)

No need to do this conditionally - just unconditionally set CodeModel to M.getCodeModel() in the else since it should get None if not set on the Module.

This revision is now accepted and ready to land.Sep 21 2018, 10:42 AM
This revision was automatically updated to reflect the committed changes.