This patch adds thread_local to llvm.mlir.global and adds translation for dso_local and addr_space to and from LLVM IR.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td | ||
---|---|---|
1004 | Can we just do thread_local? I don't see a big benefit in shrinking it. |
Added test for importing from LLVM IR.
mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td | ||
---|---|---|
1004 | It is a keyword in C++ and causes compilation issues with the generated builders. Is there a way to give a custom name of the argument to the autogenerated builder without changing the name of unit attribute in MLIR? |
mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp | ||
---|---|---|
438 | I know that just like dsoLocal, this wasn't done previously, but this should set the address space from the global variable as well while we are at it, not default it to 0. |
mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp | ||
---|---|---|
1598 | These should be getThrLocalAttrName(result.name) (if LLVM Dialect has been switched to prefixed) |
mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp | ||
---|---|---|
1598 | I don't think this is an autogenerated function. Also, it isn't clear to me what this function is supposed to do. There are some functions here. Are you referring to a similar function for "thr_local"? If yes, why the result.name argument? An example usage from the tree would be helpful. |
mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td | ||
---|---|---|
1004 | We should fix ODS backend to handle the keyword, where does it cause an issue? |
mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td | ||
---|---|---|
1004 | The argument name here (thread_local) is taken as-is while putting in builder declarations and definitions. This causes compilation error because thread_local is a C++ keyword too. A solution to this would be to convert all argument names to camel-case with the first letter capitalized before using them as builder arguments. Another solution would be to prefix/suffix them with something. Currently, the generated builder signature is - build(..., bool thread_local, ...) // ^ compilation error. |
mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td | ||
---|---|---|
1004 | How about $thread_local_ ? We already have precedent with $volatile_ in this dialect. There is a switch that makes us generate accessors in camel case,getThreadLocal, but it works at the level of the dialect. We should switch it eventually, but it will create churn across the code base. | |
mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp | ||
1598 | It is an autogenerated function, and I see similar functions for different attributes in build/tools/mlir/include/mlir/Dialect/LLVMIR/LLVMOps.h.inc. result.name is necessary to access the proper part of the storage in the registry for a yet unconstructed operation (once constructed, it will be possible to call the function without it because the OperationName can be obtained from an instance of Operation *). We shouldn't be using magic strings in the code. |
mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td | ||
---|---|---|
1004 |
It is already the case for this dialect, we need to handle variable names as well apparently. |
Can we just do thread_local? I don't see a big benefit in shrinking it.