This is an archive of the discontinued LLVM Phabricator instance.

Add target-specific pre-linking passes to Clang
AbandonedPublic

Authored by yaxunl on May 26 2016, 7:35 AM.

Details

Reviewers
Anastasia
rsmith
Summary

Sometimes a backend needs to apply certain target-specific passes before linking. This patch attempts to add that.

It depends on a new virtual member function addPreLinkPasses to be added to TargetMachine. The corresponding patch is here:

http://reviews.llvm.org/D20682

The link to the discussion in cfe-dev about pre-linking passes is here:

http://lists.llvm.org/pipermail/cfe-dev/2016-May/048822.html

Diff Detail

Event Timeline

yaxunl updated this revision to Diff 58609.May 26 2016, 7:35 AM
yaxunl retitled this revision from to Add target-specific pre-linking passes to Clang.
yaxunl updated this object.
yaxunl added reviewers: rsmith, Anastasia.
yaxunl added subscribers: cfe-commits, tstellarAMD.
yaxunl updated this object.May 26 2016, 7:44 AM

Can you give some examples of what pre-link passes may be required?

yaxunl updated this object.May 26 2016, 11:15 AM

+Jeff

To cite some of the previous discussions (http://lists.llvm.org/pipermail/cfe-dev/2016-May/048822.html )

Brian:

On our side, we use such pre-link passes to interface with the specifics of our built-in function library. For example, we transform printf calls into a form that interacts with our library. We also transform calls to pipe functions, kernel enqueue related functions, and transform calls to the atomic functions to corresponding LLVM atomic instructions (and keep track of the memory scope elsewhere currently). You may have noticed that we have a proposal out to enable the atomic instructions to directly carry a memory scope.

Jeff:

We have similar passes related to builtin functions but they are rather specific to our implementation and not too complex.

Thanks.

Sam

Anastasia edited edge metadata.Jun 3 2016, 10:06 AM

Do you think we could add any test for this change?

lib/CodeGen/BackendUtil.cpp
657

Remove the empty line please

lib/CodeGen/CodeGenAction.cpp
169

Is there any reason for having this as a callback now?

Could we just add a call to prelink passes here above instead without modifying much the original flow?

yaxunl marked 2 inline comments as done.Jun 6 2016, 12:32 PM

Do you think we could add any test for this change?

We have prelinking passes in amdgpu backend but it requires the llvm change to be committed first. We can add a test for this after that.

lib/CodeGen/CodeGenAction.cpp
169

EmitBackendOutput does not set its own diagnostic handler. When linking error happens, the diagnostic handler of BackendConsumer is used, which requires the member variable CurLinkModule of BackendConsumer to be set to current module to be linked to emit correct error msg. Therefore the linking step of EmitBackendOutput needs to update a member of BackendConsumer, a lambda function can achieve that with minimal change to other parts of the code.

An alternative implementation can do without the lambda function, but needs to

  1. define a diagnostic handler for EmitBackendOutput
  2. at the begining of EmitBackendOutput, save the old diagnostic handler and set the new one
  3. at the end of EmitBackendOutput, recover the old diagnostic handler
  4. define a helper class for diagnostic handler for EmitBackendOutput to retain the states needed for emitting diagnostic msgs
  5. move or copy the diagnostic handling required by EmitBackendOutput from the diagnostic handler of BackendConsumer to the helper class of diagnostic handler for EmitBackendOutput

Do we want to take this approach?

Do you think we could add any test for this change?

We have prelinking passes in amdgpu backend but it requires the llvm change to be committed first. We can add a test for this after that.

Sure. Could you subscribe me to the relevant backend reviews if possible please? Thanks!

lib/CodeGen/CodeGenAction.cpp
169

I see, seems complicated indeed. Would returning the module into CurLinkModule be possible instead?

yaxunl marked 2 inline comments as done.Jun 8 2016, 12:48 PM

We have prelinking passes in amdgpu backend but it requires the llvm change to be committed first. We can add a test for this after that.

Sure. Could you subscribe me to the relevant backend reviews if possible please? Thanks!

Sure, will do when we review such a pass.

lib/CodeGen/CodeGenAction.cpp
169

We can replace the lambda function argument with two arguments: the modules to be linked with, and a pointer to CurLinkModule. Then CurLinkModule can be updated during linking.

There may be a more elegant way. I will see if I can add a new function for performing pre-linking passes and call it from HandleTranslationUnit. Then we can keep the linking stage in HandleTranslationUnit unchanged.

yaxunl updated this revision to Diff 60105.Jun 8 2016, 2:41 PM
yaxunl edited edge metadata.
yaxunl marked an inline comment as done.

Refactor to eliminate lambda function argument.

LGTM! Looks much cleaner now. Please, add me to related reviews later on.

Thanks!

Anastasia accepted this revision.Jun 10 2016, 9:05 AM
Anastasia edited edge metadata.
This revision is now accepted and ready to land.Jun 10 2016, 9:05 AM
yaxunl abandoned this revision.Aug 9 2016, 8:34 AM