This is an archive of the discontinued LLVM Phabricator instance.

llvm-lto: default Relocation Model should be selected by the TargetMachine.
ClosedPublic

Authored by w2yehia on Feb 25 2021, 3:57 PM.

Details

Summary

Right now, the createTargetMachine function in LTOBackend.cpp (used by llvm-lto, and other components) selects the default Relocation Model when none is specified in the module.
Other components (such as opt and llc) that construct a TargetMachine delegate the decision on the default value to the polymorphic TargetMachine's constructor.

This commit aligns llvm-lto with other components.

Diff Detail

Event Timeline

w2yehia created this revision.Feb 25 2021, 3:57 PM
w2yehia requested review of this revision.Feb 25 2021, 3:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 25 2021, 3:57 PM
w2yehia edited the summary of this revision. (Show Details)Feb 25 2021, 4:10 PM
daltenty accepted this revision.Feb 26 2021, 8:19 AM

LGTM, thanks!

Noting this is to fix assertions in llvm/test/LTO/X86/remangle_intrinsics.ll on AIX.

This revision is now accepted and ready to land.Feb 26 2021, 8:19 AM
fhahn added a comment.Mar 2 2021, 2:50 PM

Any chance you could add an explicit test?

@fhahn do you mean a negative test to make sure the assertion that got tripped in llvm/test/LTO/X86/remangle_intrinsics.ll does not happen anymore? Or test that llvm-lto delegates the relocation model property to the TargetMachine?

w2yehia updated this revision to Diff 329514.Mar 9 2021, 7:21 PM

@daltenty provided a reduced testcase that would fail without this fix.

@daltenty provided a reduced testcase that would fail without this fix.

Thanks!

llvm/test/tools/llvm-lto/aix.ll
4 ↗(On Diff #329514)

this needs a "REQUIRES" line to make sure the powerpc target has been built.

w2yehia updated this revision to Diff 329672.Mar 10 2021, 8:13 AM
w2yehia added inline comments.
llvm/test/tools/llvm-lto/aix.ll
4 ↗(On Diff #329514)

good point, thank you.

fhahn accepted this revision.Mar 10 2021, 8:19 AM

LGTM, thanks!

This revision was landed with ongoing or failed builds.Mar 10 2021, 2:31 PM
This revision was automatically updated to reflect the committed changes.

Shouldn't the test be in llvm/test/... instead of test/...? This seems like a mistake.

Shouldn't the test be in llvm/test/... instead of test/...? This seems like a mistake.

oops, will fix it. Thank you for pointing out.