Page MenuHomePhabricator

[HCC] Add flag to Import Weak Functions in Function Importer
Needs ReviewPublic

Authored by ashi1 on Nov 7 2017, 9:10 AM.

Details

Reviewers
yaxunl
b-sumner
Summary

For HCC and HIP, in order to run kernels on our GPUs, we need everything to be inlined. We are moving towards ThinLTO implementation of the Link-time. To take advantage of ThinLTO's cross-module function importing, we must have all functions imported to kernel files including weak functions.

Since Function Importing prevents importing weak functions due to ambiguity, we would like to set a flag which can force weak functions to be imported as well.

Diff Detail

Repository
rL LLVM

Event Timeline

ashi1 created this revision.Nov 7 2017, 9:10 AM
whchung added a subscriber: whchung.Nov 7 2017, 9:14 AM
yaxunl edited edge metadata.Nov 7 2017, 9:27 AM

Could you please upload a diff with full context? Also, need a lit test.

ashi1 updated this revision to Diff 122271.Nov 9 2017, 10:44 AM
ashi1 added subscribers: scchan, yaxunl, ashi1.

I've added the lit tests for this change, and also showing full context.

My lit test import_weak_type.ll follows similar format to import_opaque_type.ll.

yaxunl added inline comments.Dec 4 2017, 8:00 AM
lib/Transforms/IPO/FunctionImport.cpp
107

Is it possible not to expose this option through extern? Generally these options should be kept static.

AlexVlx added inline comments.Dec 4 2017, 8:16 AM
lib/Transforms/IPO/FunctionImport.cpp
107

This seems to suggest that it is not utterly unacceptable: http://llvm.org/docs/CommandLine.html#internal-vs-external-storage, and the use case described there maps pretty closely to this, since the flag itself must be accessible from two different TUs. Am I missing something?

yaxunl added inline comments.Dec 5 2017, 10:09 AM
lib/Transforms/IPO/FunctionImport.cpp
107

OK. I did not notice that it is also used byFunctionImportUtils.cpp.

lib/Transforms/Utils/FunctionImportUtils.cpp
153

If we link three modules, A, B, and C.

A calls function sin, B and C both contains function sin with weak linkage.

Which sin function will be linked?

b-sumner edited edge metadata.Dec 5 2017, 10:19 AM

The usual rule is to take the first weak definition encountered.

The usual rule is to take the first weak definition encountered.

Will this work for us? Usually we would like the last one to be linked.

ashi1 added a comment.Mar 22 2018, 7:55 AM

Is first one encountered a poor design?

Is first one encountered a poor design?

Strong or first weak is the standard behavior for ISA level linkers.