This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU Device Libs pass.
AbandonedPublic

Authored by nhaustov on Jul 25 2016, 5:45 AM.

Details

Summary

Replace calls to __oclc_option_mask in a kernel.

If there is call to __oclc_option_mask in a function,
add option mask as argument and pass it through all calls to this function.

Diff Detail

Event Timeline

nhaustov updated this revision to Diff 65331.Jul 25 2016, 5:45 AM
nhaustov retitled this revision from to AMDGPU Device Libs pass..
nhaustov updated this object.
nhaustov added a subscriber: llvm-commits.
lib/Target/AMDGPU/AMDGPUDeviceLibs.h
2

Header comment is missing.

6

Can you add a comment explaining what this option is for.

lib/Target/AMDGPU/AMDGPUDeviceLibsPass.cpp
2

This file needs a header comment with an explanation of what it does.

39

Do we want to set this even at -O0 ?

58

Coding style: brace shouldn't be on its own line.

77

Coding Style.

91

Coding Style.

138

Coding Style.

arsenm edited edge metadata.Jul 25 2016, 10:38 AM

Should this really be in the backend? I also thought we were going to use a global constant variable instead of a function call

Should this really be in the backend? I also thought we were going to use a global constant variable instead of a function call

Yes, it needs to be. Global variable approach doesn't support functions in different modes.
Any specific concerns with the patch?

Should this really be in the backend? I also thought we were going to use a global constant variable instead of a function call

Yes, it needs to be. Global variable approach doesn't support functions in different modes.
Any specific concerns with the patch?

The idea was that would be controlled by multiple globals with private visibility in different modules linked together.
Why does it need to be in the backend? This seems more like a pass that belongs with the runtime/compiler library when linking the AMD specific library

Should this really be in the backend? I also thought we were going to use a global constant variable instead of a function call

Yes, it needs to be. Global variable approach doesn't support functions in different modes.
Any specific concerns with the patch?

The idea was that would be controlled by multiple globals with private visibility in different modules linked together.
Why does it need to be in the backend? This seems more like a pass that belongs with the runtime/compiler library when linking the AMD specific library

Multiple variables would not work with function calls (see example with call in the review).

Yes, it will be run before backend passes (in pre-link phase). It's common for all languages though.

nhaustov updated this revision to Diff 65485.Jul 26 2016, 3:21 AM
nhaustov edited edge metadata.

Update after Tom's review.

nhaustov marked 8 inline comments as done.Jul 26 2016, 3:22 AM
nhaustov added inline comments.
lib/Target/AMDGPU/AMDGPUDeviceLibsPass.cpp
40

Since the linking is at bitcode level, we can. We should review it later.

yaxunl edited edge metadata.Jul 26 2016, 2:42 PM

Can we have a test like this:

int f() {
  if (__oclc_option_mask() & X)
    return 1
  else if (__oclc_option_mask() & Y)
    return 2;
}

then call it in different kernels.

test/CodeGen/AMDGPU/device-libs.ll
45

Can we have another kernel calling get_option_mask2() with different function attributes?

arsenm added inline comments.Jul 26 2016, 4:11 PM
lib/Target/AMDGPU/AMDGPUDeviceLibsPass.cpp
60–65

if bodies on separate lines

143–147

Why do we have to handle this GC bit? needs tests

164–181

Should make common with CallSite. Why does invoke need to be handled at all? No library function should ever throw

205–206

pop_back_val?

nhaustov updated this revision to Diff 65731.Jul 27 2016, 6:55 AM
nhaustov marked 6 inline comments as done.
nhaustov edited edge metadata.
  • Update after Matt's review.
lib/Target/AMDGPU/AMDGPUDeviceLibsPass.cpp
60–65

Rewrote.

143–147

Removed.

164–181

What do you mean 'common with CallSite'?
I've removed invoke.

205–206

It's std::set which doesn't have it.

I am thinking probably this should be a post-linking pass.

The library functions can be identified by prefixes or name mangling. All OpenCL builtin functions start with _Z ( IA64 mangled ) and other library functions have specific prefixes. We add an extra option argument to all library functions. If a library function calls other library functions, pass this option argument down.

Non-library functions are compiled with cl- options which become function attributes. We just need to translate them to option mask and pass it to the OpenCL buitin functions called by this non-library function.

After this transformation, all library function calls will have an option argument equivalent to the cl- options they are compiled with in the original compilation unit.

nhaustov abandoned this revision.Aug 1 2016, 2:11 AM

Due to difficulties, it was decided to pursue different approach - create control functions in Runtime and link them as bitcode libraries.