Page MenuHomePhabricator

Initializing NVPTX backend within Polly
ClosedPublic

Authored by singam-sanjay on Apr 9 2017, 2:22 AM.

Details

Summary

The NVPTX backend is now initialised within Polly. A language front-end need not be modified to initialise the backend, just for Polly.

Diff Detail

Repository
rL LLVM

Event Timeline

singam-sanjay created this revision.Apr 9 2017, 2:22 AM
Meinersbur edited edge metadata.Apr 10 2017, 7:02 AM

With -DLLVM_TARGETS_TO_BUILD=X86 (i.e. without nvptx backend) I get this error:

FAILED: /usr/bin/c++   -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Itools/polly/lib -I/root/src/llvm/tools/polly/lib -Itools/polly/include -I/root/src/llvm/tools/polly/lib/External -I/root/src/llvm/tools/polly/lib/External/pet/include -I/root/src/llvm/tools/polly/lib/External/JSON/include -I/root/src/llvm/tools/polly/lib/External/isl/include -Itools/polly/lib/External/isl/include -I/root/src/llvm/tools/polly/include -Iinclude -I/root/src/llvm/include -fPIC -fvisibility-inlines-hidden -Werror=date-time -std=c++11 -Wall -W -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wno-maybe-uninitialized -Wdelete-non-virtual-dtor -Wno-comment -ffunction-sections -fdata-sections -fno-common -Woverloaded-virtual -Wno-long-long -Wall -W -Wno-unused-parameter -Wwrite-strings -fno-exceptions -fno-rtti -O3    -UNDEBUG -MMD -MT tools/polly/lib/CMakeFiles/Polly.dir/Support/RegisterPasses.cpp.o -MF tools/polly/lib/CMakeFiles/Polly.dir/Support/RegisterPasses.cpp.o.d -o tools/polly/lib/CMakeFiles/Polly.dir/Support/RegisterPasses.cpp.o -c /root/src/llvm/tools/polly/lib/Support/RegisterPasses.cpp
/root/src/llvm/tools/polly/lib/Support/RegisterPasses.cpp: In function ‘void polly::initializePollyPasses(llvm::PassRegistry&)’:
/root/src/llvm/tools/polly/lib/Support/RegisterPasses.cpp:210:31: error: ‘LLVMInitializeNVPTXTarget’ was not declared in this scope
     LLVMInitializeNVPTXTarget();
                               ^
/root/src/llvm/tools/polly/lib/Support/RegisterPasses.cpp:211:35: error: ‘LLVMInitializeNVPTXTargetInfo’ was not declared in this scope
     LLVMInitializeNVPTXTargetInfo();
                                   ^
/root/src/llvm/tools/polly/lib/Support/RegisterPasses.cpp:212:33: error: ‘LLVMInitializeNVPTXTargetMC’ was not declared in this scope
     LLVMInitializeNVPTXTargetMC();
                                 ^
/root/src/llvm/tools/polly/lib/Support/RegisterPasses.cpp:213:35: error: ‘LLVMInitializeNVPTXAsmPrinter’ was not declared in this scope
     LLVMInitializeNVPTXAsmPrinter();
                                   ^
singam-sanjay added a comment.EditedApr 10 2017, 10:01 AM

If there were calls to LLVMInitializeNVPTX*(), then GPU_CODEGEN was defined without building the NVPTX backend. Can you check if ENABLE_POLLY_GPU_CODEGEN set to ON with only X86 being built ?

If there were calls to LLVMInitializeNVPTX*(), then GPU_CODEGEN was defined without building the NVPTX backend. Can you check if ENABLE_POLLY_GPU_CODEGEN set to ON with only X86 being built ?

ENABLE_POLLY_GPU_CODEGEN=ON in my case. If that setting requires ENABLE_POLLY_GPU_CODEGEN=NVPTX, it should be checked in CMakeLists.txt, instead of failing randomly when compiling.

Note that we might add an OpenCL/SPIR backend to Polly-ACC in the future that does not require the NVPTX backend.

Stop build configuration if NVPTX not being built.

singam-sanjay added a comment.EditedApr 11 2017, 1:33 AM

Note that we might add an OpenCL/SPIR backend to Polly-ACC in the future that does not require the NVPTX backend.

The modifications to integrate the OpenCL backend would depend on whether we would let Polly choose between backends at runtime or compile time.

i.e. the difference between having "-polly-target=gpu_ocl or -polly-target=gpu_cuda" and just "-polly-target=gpu" with one of the GPU backends being built.

We could even try to figure the type of GPU installed and build/use the appropriate backend.

Using -DPOLLY_ENABLE_GPGPU_CODEGEN=ON -DLLVM_POLLY_LINK_INTO_TOOLS=OFF I get (ninja check-polly):

opt: CommandLine Error: Option 'nvptx-emit-line-numbers' registered more than once!

Using -DPOLLY_ENABLE_GPGPU_CODEGEN=ON -DLLVM_BUILD_LLVM_DYLIB=ON -DLLVM_LINK_LLVM_DYLIB=ON or -DPOLLY_ENABLE_GPGPU_CODEGEN=ON -DBUILD_SHARED_LIBS=ON I get:

opt: Unknown command line argument '-polly-codegen-ppcg'.  Try: 'opt -help'
opt: Did you mean '-polly-codegen'?

@grosser The build stops when the LLVMInitializeNVPTX*() functions aren't found. These are made available by linking the libraries, which cannot by done by including header files.

/root/src/llvm/tools/polly/lib/Support/RegisterPasses.cpp: In function ‘void polly::initializePollyPasses(llvm::PassRegistry&)’:
/root/src/llvm/tools/polly/lib/Support/RegisterPasses.cpp:210:31: error: ‘LLVMInitializeNVPTXTarget’ was not declared in this scope

LLVMInitializeNVPTXTarget();
                          ^

/root/src/llvm/tools/polly/lib/Support/RegisterPasses.cpp:211:35: error: ‘LLVMInitializeNVPTXTargetInfo’ was not declared in this scope

LLVMInitializeNVPTXTargetInfo();
                              ^

/root/src/llvm/tools/polly/lib/Support/RegisterPasses.cpp:212:33: error: ‘LLVMInitializeNVPTXTargetMC’ was not declared in this scope

LLVMInitializeNVPTXTargetMC();
                            ^

/root/src/llvm/tools/polly/lib/Support/RegisterPasses.cpp:213:35: error: ‘LLVMInitializeNVPTXAsmPrinter’ was not declared in this scope

LLVMInitializeNVPTXAsmPrinter();
                              ^

Answering your question here,

On Wed, 12 Apr 2017 at 00:36 Tobias Grosser wrote:
How do the header file requirements influence what we link into Polly?
What will break if we do not change the linking?

Best,
Tobias

On Tue, Apr 11, 2017, at 08:07 PM, Sanjay Srivallabh Singapuram wrote:
That's not working because the function definitions aren't available to
Polly through the headers.

On Tue 11 Apr, 2017, 11:35 PM Tobias Grosser wrote:

I don't think we need to change anything with the linking. We should
just call the functions when needed.

Best,
Tobias

Could we maybe move the responsibility to initialize the NVPTX target to the host application? opt and clang do this already by calling LLVMInitializeAllTargets.

What exactly is causing each of these issues ?

Using -DPOLLY_ENABLE_GPGPU_CODEGEN=ON -DLLVM_POLLY_LINK_INTO_TOOLS=OFF I get (ninja check-polly):

opt: CommandLine Error: Option 'nvptx-emit-line-numbers' registered more than once!

I guess here the problem is with the redeclaration of EmitLineNumbers. This isn't an issue when Polly is linked into tools because only one copy of LLVMNVPTX* libraries is linked into the program, either Polly's or the ones linked through the command line. Please correct me if I'm wrong.

only
Using -DPOLLY_ENABLE_GPGPU_CODEGEN=ON -DLLVM_BUILD_LLVM_DYLIB=ON -DLLVM_LINK_LLVM_DYLIB=ON or -DPOLLY_ENABLE_GPGPU_CODEGEN=ON -DBUILD_SHARED_LIBS=ON I get:

opt: Unknown command line argument '-polly-codegen-ppcg'.  Try: 'opt -help'
opt: Did you mean '-polly-codegen'?

Even I faced this when I ran ninja check-polly. I couldn't understand why this was happening since -polly-codegen-gen wasn't declared as a cl::opt.

Could we maybe move the responsibility to initialize the NVPTX target to the host application? opt and clang do this already by calling LLVMInitializeAllTargets.

This patch was meant to ease Polly's integration into frontends by taking care of the backend initialization. E.g. Julia developers weren't comfortable with calling LLVMInitializeAllTargets (https://github.com/JuliaLang/julia/pull/21142/commits/3371d635da12444f81034ca02a668fe9cefd7388#r107699287). So, the onus to initialize the backend now falls on Polly. For Julia it could be done in a module dedicated for Polly, but the effort still has to be repeated for every frontend that integrates with Polly.

I tried that first. But it required initializing all backends and hence linking all backends.

On Wed, 12 Apr 2017 at 09:50 Tobias @grosser wrote

Right. This would be the best option from our perspective, but to easy
integration of Polly into
other tools it seems OK to call the initialization from Polly. Can you
just call initializeAllTargets from Polly without any of the linking
changes?

Could we maybe move the responsibility to initialize the NVPTX target to the host application? opt and clang do this already by calling LLVMInitializeAllTargets.

This patch was meant to ease Polly's integration into frontends by taking care of the backend initialization. E.g. Julia developers weren't comfortable with calling LLVMInitializeAllTargets (https://github.com/JuliaLang/julia/pull/21142/commits/3371d635da12444f81034ca02a668fe9cefd7388#r107699287). So, the onus to initialize the backend now falls on Polly. For Julia it could be done in a module dedicated for Polly, but the effort still has to be repeated for every frontend that integrates with Polly.

On Wed, 12 Apr 2017 at 11:12 @grosser wrote:

Which errors did you get?

For this edit in RegisterPasses.cpp,

#ifdef GPU_CODEGEN
  if (Target == TARGET_GPU) {
    initializePPCGCodeGenerationPass(Registry);
    InitializeAllTargets();
    /*LLVMInitializeNVPTXTarget();
    LLVMInitializeNVPTXTargetInfo();
    LLVMInitializeNVPTXTargetMC();
    LLVMInitializeNVPTXAsmPrinter();*/
  }
#endif

I get the following error when I don't link the NVPTX libraries.

[280/280] Linking CXX executable bin/bugpoint
FAILED: : && /usr/bin/c++   -fPIC -fvisibility-inlines-hidden -std=c++11 -Wall -W -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wno-maybe-uninitialized -Wdelete-non-virtual-dtor -Wno-comment -ffunction-sections -fdata-sections -O3 -DNDEBUG  -Wl,-allow-shlib-undefined   -Wl,--export-dynamic  -Wl,-O3 -Wl,--gc-sections tools/bugpoint/CMakeFiles/bugpoint.dir/BugDriver.cpp.o tools/bugpoint/CMakeFiles/bugpoint.dir/CrashDebugger.cpp.o tools/bugpoint/CMakeFiles/bugpoint.dir/ExecutionDriver.cpp.o tools/bugpoint/CMakeFiles/bugpoint.dir/ExtractFunction.cpp.o tools/bugpoint/CMakeFiles/bugpoint.dir/FindBugs.cpp.o tools/bugpoint/CMakeFiles/bugpoint.dir/Miscompilation.cpp.o tools/bugpoint/CMakeFiles/bugpoint.dir/OptimizerDriver.cpp.o tools/bugpoint/CMakeFiles/bugpoint.dir/ToolRunner.cpp.o tools/bugpoint/CMakeFiles/bugpoint.dir/bugpoint.cpp.o  -o bin/bugpoint  lib/libLLVMAnalysis.a lib/libLLVMBitWriter.a lib/libLLVMCodeGen.a lib/libLLVMCore.a lib/libLLVMipo.a lib/libLLVMIRReader.a lib/libLLVMInstCombine.a lib/libLLVMInstrumentation.a lib/libLLVMLinker.a lib/libLLVMObjCARCOpts.a lib/libLLVMScalarOpts.a lib/libLLVMSupport.a lib/libLLVMTarget.a lib/libLLVMTransformUtils.a lib/libLLVMVectorize.a -lpthread lib/libPolly.a lib/libLLVMTarget.a lib/libLLVMBitWriter.a lib/libLLVMAsmParser.a lib/libLLVMInstCombine.a lib/libLLVMTransformUtils.a lib/libLLVMAnalysis.a lib/libLLVMObject.a lib/libLLVMBitReader.a lib/libLLVMMCParser.a lib/libLLVMMC.a lib/libLLVMProfileData.a lib/libLLVMCore.a lib/libLLVMSupport.a -lrt -ldl -ltinfo -lpthread -lz -lm lib/libLLVMDemangle.a lib/libPollyPPCG.a lib/libPollyISL.a -Wl,-rpath,"\$ORIGIN/../lib" && :
lib/libPolly.a(RegisterPasses.cpp.o):RegisterPasses.cpp:function polly::initializePollyPasses(llvm::PassRegistry&): error: undefined reference to 'LLVMInitializeX86TargetInfo'
lib/libPolly.a(RegisterPasses.cpp.o):RegisterPasses.cpp:function polly::initializePollyPasses(llvm::PassRegistry&): error: undefined reference to 'LLVMInitializeNVPTXTargetInfo'
lib/libPolly.a(RegisterPasses.cpp.o):RegisterPasses.cpp:function polly::initializePollyPasses(llvm::PassRegistry&): error: undefined reference to 'LLVMInitializeX86Target'
lib/libPolly.a(RegisterPasses.cpp.o):RegisterPasses.cpp:function polly::initializePollyPasses(llvm::PassRegistry&): error: undefined reference to 'LLVMInitializeNVPTXTarget'
collect2: error: ld returned 1 exit status
ninja: build stopped: subcommand failed.
On Wed, 12 Apr 2017 at 09:50 @grosser wrote:

The problem is that the LLVM backends are now linked twice into opt.
Each instance declares command line options and registers passes. This
does not work.

Will linking to LLVNNVPTX*.so instead of LLVMNVPTX*.a help ? We could build them just for Polly.

On Wed, 12 Apr 2017 at 17:02 @Meinersbur wrote:

I meant that both, initializing the targets and linking to be the responsibility of the host application. opt and clang do this and it obviously works. If we allow other host programs to only link and init their primary target, we have to special-case this every time.

By host-program I assume you mean the program that converts a language into LLVM-IR and orchestrates everything. So, what you're saying is it's up to the host-program to initialize and provide what Polly needs. Have I understood you correctly ?

I still think linking is not the problem here, the same library linked-in twice is resolved by the linker and loader.

The opt: CommandLine Error: Option 'nvptx-emit-line-numbers' registered more than once! error that you received here could be a link error because you were linking archives not shared libraries. I didn't receive this error during ninja check-polly when I built LLVM with -DPOLLY_ENABLE_GPGPU_CODEGEN=ON -DLLVM_POLLY_LINK_INTO_TOOLS=OFF -DBUILD_SHARED_LIBS=ON.

The problem is that opt and clang already call LLVMInitializeAllTargets which in turn initializes the NVPTX target, and we do it again.

I'm not sure is that's a problem. As a part of this discussion on the polly-dev mailing list, I checked if reinitializing the backends was a problem by calling InitializeAllTarget*() functions in a loop with 10 iterations. The only effect it had was delaying the start by few hundredths of a microsecond.

There seems to be no protection for double-initialization.

One way is to check if the NVPTX target triples are already available through TargetRegistry::lookupTarget. But, I'm not sure if the availability of the target triples means that LLVMInitialize*TargetMC and LLVMInitialize*AsmPrinter were called.

I think Tobias is right and this is more of a linking problem. It starts with bugpoint to not depend on the llvm targets (It instead calls opt to do the main work, itself it only needs to know which passes are available). Polly-ACC seems to be the only pass that has a link-dependency against a specific backend.

I suggested to Sanjay to create a patch that links bugpoint against the backends, just like opt does. bugpoint is an LLVM-developer only tool so there shouldn't be a lot of resistance against that. If there is, we can use weak symbols so linking does not fail in bugpoint (and its never used in bugpoint).

I suggested to Sanjay to create a patch that links bugpoint against the backends, just like opt does. bugpoint is an LLVM-developer only tool so there shouldn't be a lot of resistance against that. If there is, we can use weak symbols so linking does not fail in bugpoint (and its never used in bugpoint).

I've made these changes to <llvm_src>/tools/bugpoint/CMakeLists.txt

-if(WITH_POLLY AND LINK_POLLY_INTO_TOOLS)
-  target_link_libraries(bugpoint Polly)
-  # Ensure LLVMTarget can resolve dependences in Polly.
-  target_link_libraries(bugpoint LLVMTarget)
-endif(WITH_POLLY AND LINK_POLLY_INTO_TOOLS)
+function(chk_GPU_lib_for_Polly lib_name)
+  if (NOT TARGET ${lib_name})
+    message(FATAL_ERROR 
+            "${lib_name} not being built, is required by Polly-ACC.")
+  endif (NOT TARGET ${lib_name})
+endfunction(chk_GPU_lib_for_Polly)
+
+if (WITH_POLLY)
+  if (LINK_POLLY_INTO_TOOLS)
+    target_link_libraries(bugpoint Polly)
+    # Ensure LLVMTarget can resolve dependences in Polly.
+    target_link_libraries(bugpoint LLVMTarget)
+  endif (LINK_POLLY_INTO_TOOLS)
+  if (POLLY_ENABLE_GPGPU_CODEGEN)
+    chk_GPU_lib_for_Polly(LLVMNVPTXCodeGen)
+    chk_GPU_lib_for_Polly(LLVMNVPTXInfo)
+    chk_GPU_lib_for_Polly(LLVMNVPTXDesc)
+    chk_GPU_lib_for_Polly(LLVMNVPTXAsmPrinter)
+    LIST(APPEND LLVM_LINK_COMPONENTS
+        NVPTX
+        #LLVMNVPTXCodeGen
+        #LLVMNVPTXInfo
+        #LLVMNVPTXDesc
+        #LLVMNVPTXAsmPrinter
+       )

When POLLY_ENABLE_GPGPU_CODEGEN is enabled, NVPTX links to bugpoint irrespective of whether Polly is linked into bugpoint. Does this fall under LLVM's design principles ?

singam-sanjay updated this revision to Diff 95085.EditedApr 13 2017, 12:41 AM

Linking NVPTX into Polly causes problem when NVPTX is linked separately e.g. opt and clang already link all backends, including NVPTX. This removes the onus of linking NVPTX from Polly and assumes that tools that link to Polly also take care of linking NVPTX. D32003 is one such change to LLVM's bugpoint.

Unconditionally initializing the NVPTX backend since the Target variable is still isn't assigned to the value passed as a command line option when initializePollyPasses is called in bugpoint.cpp.

Using target_link_libraries( Polly INTERFACE ... ) to prevent linking twice and resolve all dependences within Polly.

With -DLLVM_POLLY_LINK_INTO_TOOLS=OFF (everything else left as default) I get:

opt: CommandLine Error: Option 'nvptx-emit-line-numbers' registered more than once!

I don't think target_link_libraries(Polly INTERFACE ...) solves our problems.

lib/CMakeLists.txt
70 ↗(On Diff #96114)

With an out-of-LLVM source tree I get

CMake Error at lib/CMakeLists.txt:70 (llvm_map_components_to_libnames):
  Unknown CMake command "llvm_map_components_to_libnames".

With -DLLVM_POLLY_LINK_INTO_TOOLS=OFF (everything else left as default) I get:

opt: CommandLine Error: Option 'nvptx-emit-line-numbers' registered more than once!

I don't think target_link_libraries(Polly INTERFACE ...) solves our problems.

I have a patch that's working with -DLLVM_POLLY_LINK_INTO_TOOLS=OFF. I'm testing it right now with this https://gist.github.com/singam-sanjay/7b6907c9b31385239f62c2e24068c53f script in different build configurations.

lib/CMakeLists.txt
70 ↗(On Diff #96114)

Then, listing all the NVPTX libraries would be required to work in all cases.

Would it work if Polly had its own copy of "llvm_map_components_to_libnames" ?

The patch I was talking about didn't pass.

I see that when LLVM_POLLY_LINK_INTO_TOOLS=OFF, opt uses Polly from LLVMPolly.so which pulls in the NVPTX libraries when libLLVMNVPTX*.so or libLLVM.so aren't available. The error occurs when both the executable (opt in this case) and LLVMPolly.so have a copy of the same library. Is there a way to selectively remove libraries from LLVMPolly.so after it's linked to libPolly.a and its dependences ?

Also, what are benefits of not linking Polly into tools ?

Is there a way to selectively remove libraries from LLVMPolly.so after it's linked to libPolly.a and its dependences ?

No, there isn't. It is the reason why there is no target_link_libraries(Polly LLVMCore LLVMSupport ...).

Also, what are benefits of not linking Polly into tools ?

First, you may not want to get the additional complexity of having Polly in your clang only because you checked out its source. This is much less of an argument since LLVM_POLLY_LINK_INTO_TOOLS=ON became the default.

However, this is irrelevant as long as we have the LLVMPolly.so` loadable module (which allows using Polly in a precompiled clang) which still needs to work. Without the switch LLVM_POLLY_LINK_INTO_TOOLS, we cannot easily test whether it is still working.

lib/CMakeLists.txt
70 ↗(On Diff #96114)

One might use the include command to import that function, but we get into trouble with out-of-llvm-tree builds. I don't think it is worth the hassle.

It is the reason why there is no target_link_libraries(Polly LLVMCore LLVMSupport ...).

I didn't get you.

However, this is irrelevant as long as we have the LLVMPolly.so` loadable module (which allows using Polly in a precompiled clang) which still needs to work. Without the switch LLVM_POLLY_LINK_INTO_TOOLS, we cannot easily test whether it is still working.

Is it required for a Windows build ?

Linking NVPTX now taken care of by D32442

Meinersbur accepted this revision.Apr 28 2017, 2:48 AM
This revision is now accepted and ready to land.Apr 28 2017, 2:48 AM
grosser resigned from this revision.May 6 2017, 5:24 AM

Hi Sanjay,

it seems this patch is not needed any more. You might want to close this review.

Hi Sanjay,

it seems this patch is not needed any more. You might want to close this review.

Could you point me to a commit ? I'm not able to find any recent commits in Polly.

singam-sanjay closed this revision.May 6 2017, 5:37 AM

Hi Sanjay,

it seems this patch is not needed any more. You might want to close this review.

@Meinersbur Can you please explain why this patch is unnecessary ?

Did you mean @grosser? I was under the impression that calling LLVMInitializeNVPTX* was necessary for Julia, so I am as well a bit surprised that it is not necessary anymore.

@grosser Did you notice that my patch to the Polly build system does not call LLVMInitializeNVPTX* yet. It only ensures that the NVPTX libraries are available, which is required for any LLVMInitializeNVPTX* to not result in a link error.

Did you mean @grosser?

I addressed this to you since @grosser had resigned and assumed that he wouldn't be getting updates.

I was under the impression that calling LLVMInitializeNVPTX* was necessary for Julia, so I am as well a bit surprised that it is not necessary anymore.

It is necessary for Julia.

@grosser Did you notice that my patch to the Polly build system does not call LLVMInitializeNVPTX* yet. It only ensures that the NVPTX libraries are available, which is required for any LLVMInitializeNVPTX* to not result in a link error.

This is reassuring.

After you closed the revision, I was assuming you agreed with @grosser.

singam-sanjay reopened this revision.May 10 2017, 7:23 AM
This revision is now accepted and ready to land.May 10 2017, 7:23 AM

Sorry, I misread the last message. I understand this patch is indeed needed. I am fine for it to be committed as it is. I leave the last word to Michael, as he already started the review.

I already accepted the patch as-is.

@singam-sanjay : This patch is already marked as accepted. Feel free to commit it.

This revision was automatically updated to reflect the committed changes.

@grosser @lattner I was able to commit this patch.

Thanks for your help !!