Page MenuHomePhabricator

[mlir] Avoid building some shared libraries when PIC is off

Authored by zhuhan0 on Jan 30 2023, 3:46 PM.



When LLVM_ENABLE_PIC = OFF, shared libraries cannot be built against code that's compiled without -fPIC. Example error message:

ld.lld: error: relocation R_X86_64_32 cannot be used against local symbol; recompile with -fPIC
>>> defined in lib/libLLVMSupport.a(StringMap.cpp.o)
>>> referenced by StringMap.cpp
>>>               StringMap.cpp.o:(llvm::StringMapImpl::StringMapImpl(unsigned int, unsigned int)) in archive lib/libLLVMSupport.a

Similar to how libclang handles this, skip building these shared libraries when LLVM_ENABLE_PIC = OFF.

Diff Detail

Event Timeline

zhuhan0 created this revision.Jan 30 2023, 3:46 PM
zhuhan0 requested review of this revision.Jan 30 2023, 3:46 PM
zhuhan0 updated this revision to Diff 493452.Jan 30 2023, 4:58 PM

mlir_runner_utils can still be built

Can I get a review on this?

MatzeB added inline comments.Feb 6 2023, 10:30 AM

There's other libraries like this one marked with SHARED too. What's different about this one? Not linking against LLVM?

Is the MLIR Exectutiong engine still useful without the two libs excluded here? Otherwise there is already a if(NOT MLIR_ENABLE_EXECUTION_ENGINE) return() endif() above which could be used to just disable it all if LLVM_ENABLE_PIC is false...

zhuhan0 added inline comments.Feb 6 2023, 10:52 AM

Yes the difference about this one is that it doesn't link against other code, which is built without -fPIC, so this one builds fine.

Let me see if we can disable the execution engine altogether with PIC=OFF.

zhuhan0 updated this revision to Diff 495997.Feb 8 2023, 6:25 PM
zhuhan0 edited the summary of this revision. (Show Details)

Make it cleaner. I didn't disable MLIRExecutionEngine altogether, but just added an early return as soon as anything related to shared library starts. MLIRExecutionEngine and MLIRJitRunner do not link against any shared library, so they should still be useful to stay.

So, as a note, having that early return feels like a great way for someone to add a new utility library to the wrong part of the file. Would it make more sense to make that an if and add an explanatory comment?

zhuhan0 updated this revision to Diff 496565.Feb 10 2023, 11:20 AM

Good point. Make it an if and add comment.

krzysz00 accepted this revision.Feb 13 2023, 7:37 AM

Seems like a reasonable un-breaking of the build, approved

This revision is now accepted and ready to land.Feb 13 2023, 7:37 AM

Thanks @MatzeB and @krzysz00 for the review!