This is an archive of the discontinued LLVM Phabricator instance.

[NVPTX] Added an option to run NVVMReflect pass.
ClosedPublic

Authored by tra on Jul 30 2015, 2:48 PM.

Details

Diff Detail

Event Timeline

tra updated this revision to Diff 31076.Jul 30 2015, 2:48 PM
tra retitled this revision from to [NVPTX] Added an option to run NVVMReflect pass..
tra updated this object.
tra added reviewers: echristo, jholewinski.
tra added a subscriber: llvm-commits.
echristo edited edge metadata.Aug 24 2015, 1:18 PM

Testcase to make sure it's running?

-eric

tra updated this revision to Diff 33001.Aug 24 2015, 2:46 PM
tra edited edge metadata.

Added test case to verify that nvptx-enable-reflect option works.

I guess that's one way. I was hoping for a basic functionality test?

-eric

tra added a comment.EditedSep 8 2015, 10:51 AM

I guess that's one way. I was hoping for a basic functionality test?

NVVMReflect pass functionality is already tested in test/CodeGen/NVPTX/nvvm-reflect.ll

In addition, tests in D11664 (clang-side counterpart of this patch) verify end-to-end functionality which ensures that __nvvm_reflect() is eliminated when -nvptx-enable-reflect option is passed.

Seems this could be unified with with the nvvm-reflect-enable option in the pass itself? Actually, is there any reason this shouldn't be on by default for the backend? (i.e. am I missing something here?)

-eric

tra added a comment.Sep 8 2015, 1:04 PM

Seems this could be unified with with the nvvm-reflect-enable option in the pass itself? Actually, is there any reason this shouldn't be on by default for the backend? (i.e. am I missing something here?)

Your point makes sense as linking with libdevice will be the common case for compiling real CUDA apps.
Adding NVVMReflect pass unconditionally would break whoever may want to compile their own libdevice variant.
It can be easily fixed with -nvvm-reflect-enable=0, so it should not be too big of a deal.

tra added a comment.Sep 8 2015, 1:13 PM

On second thought, adding NVVMReflect pass with *default* settings unconditionally may make is hard to replace it with the one that takes optional StringMap. I don't think we need that now, but if/when that happens we can make NVVMReflect pass conditional then.

tra updated this revision to Diff 34253.Sep 8 2015, 1:50 PM
tra updated this object.

Added NVVMReflect pass unconditionally.
Removed command-line option.
If NVVMReflect has to be disabled, it can be done with -nvvm-reflect-enable=0 option.

echristo accepted this revision.Sep 8 2015, 1:59 PM
echristo edited edge metadata.

LGTM. Thanks.

This revision is now accepted and ready to land.Sep 8 2015, 1:59 PM
This revision was automatically updated to reflect the committed changes.