This supports the threadprivate directive in OpenMP dialect following
the OpenMP 5.1 [2.21.2] standard. Also lowering to LLVM IR using OpenMP
IRBduiler.
Details
Diff Detail
Event Timeline
- Do we need a verifier for this?
- If yes, then we should add test cases (in invalid.mlir). Also we probably should add a test case to verify the printer in ops.mlir.
In addition to Arnamoy's comments, can we have assembly format based syntax for this? It looks like the following would work -
$sym_addr `:` type($sym_addr) `->` type($res)
Thanks for suggestions. I didn't notice the mlir format has changed. I am fixing the CI fail.
- Remove let parser/printer due to update in upstream MLIR.
- Replace syn_name() with getSymName() due to update in upstream MLIR.
- Add test case in ops.mlir
This operation def is implementation defined. I am not aware of what I should verify currently. The restrictions in OpenMP standard are not suitable to verify here, which should be in semantic analysis. Please let me know if anyone finds what should to verify.
Thanks for the patch. A few comments.
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp | ||
---|---|---|
1327 | nit: llvm::OpenMPIRBuilder::LocationDescription ompLoc(builder) also works. | |
1333 | Instead of an assert, can we return a graceful error here? | |
1340 | nit: builder.getInt8PtrTy() | |
1343 | [suggestion] Instead of exposing this function and changing the LLVM::ModuleTranslation class, we can use builder.GetInsertBlock()->getModule()->getDataLayout() here. | |
1344 | For size of types, we can use the following to get the type size directly instead of using getTypeStoreSize and getFixedSize. llvm::ConstantInt *size = Builder.getInt64(datalayout.getTypeSizeInBits(type)) |
LGTM. A few nits. Please wait for @shraiysh and @arnamoy10.
Can you add a note to the Summary saying the following or something similar?
The current implementation uses the OpenMP runtime to provide thread-local storage. We will (in future) provide support for using the TLS feature of the LLVM IR.
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td | ||
---|---|---|
896 | Nit: This operation takes in the address of a symbol that represents the original variable and returns the address of its thread local storage. All occurrences of threadprivate variables in a parallel region should use the TLS returned by this operation. | |
898 | Nit: refers to the address of the symbol, which is a pointer to the original global variable. | |
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp | ||
1333 | Nit: When the assert is removed, please replace the auto above with a type. |
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td | ||
---|---|---|
898 | Made changes: delete global. | |
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp | ||
1333 | Technically, AllocaOp works also ok here. classic-flang uses for variables in main program for fortran code. Also, OpenMP standard does not mention the variable must be global for fortran. So I add assert instead of reporting the error in verify. What do you think? @kiranchandramohan @shraiysh | |
1344 | getTypeSizeInBits is not correct. It seems it cannot handle some special data type. I followed clang using getTypeStoreSize. |
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp | ||
---|---|---|
1333 | You can report an error here itself. This error will not be inside verify, instead it will be an error while translating to LLVM IR. That allows using AllocaOp in the dialect, but reports an error only while translating to LLVM IR. Example - here. | |
1344 | I had asked about this in the discord channel and they suggested using getTypeSizeInBits to get the size of a type. Can you please specify for which types it cannot handle? |
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp | ||
---|---|---|
1333 | That makes sense. Thanks for the advice. Will fix it. | |
1344 | For example, returns 5 for i36 gives 40 in bits for i36. Clang uses that, so I use it. Using getTypeStoreSize works ok for all tested fortran data types, so I did not dig too much the difference between using getTypeStoreSize or getTypeSizeInBits. Fortran code doesn't have i36, not sure if c/c++ uses it. |
Change assert into error reporting. Remove the auto. Add the test case for the error reporting
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp | ||
---|---|---|
1344 | Okay thank you. |
Nit: This operation takes in the address of a symbol that represents the original variable and returns the address of its thread local storage. All occurrences of threadprivate variables in a parallel region should use the TLS returned by this operation.