- Convert global_memref to LLVM::GlobalOp.
- Convert get_global_memref to a memref descriptor with a pointer to the first element of the global stashed in it.
- Extend unit test and a mlir-cpu-runner test to validate the generated LLVM IR.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Nice! This is so much cleaner than my code in npcomp, which I'm excited to be able to delete now :)
I especially like how well you handled the rank-0 case. I found that one annoying, but your code handles it very cleanly.
LGTM, but probably wait for someone with more std-to-llvm experience to give a second opinion.
mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp | ||
---|---|---|
2452 | should we return failure() here if the memref type has a nontrivial address space or affine map? It looks like this code is only handling the case of a standard dense layout in the default address space. | |
2486 | I find this name a bit confusing. maybe "needs buffer size calculation"? | |
mlir/test/Conversion/StandardToLLVM/standard-to-llvm.mlir | ||
137 | nit "lvm" -> "llvm" |
mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp | ||
---|---|---|
2452 | I am now checking for affine maps (isSupportedMemRefType). Handling address space is simpler, since it just transparently goes to the LLVM::GlobalOp. (GetGlobalMemrefOpLowering already handles this). | |
2486 | I renamed the function as well as the cumulativeSize variables to just bufferSize. |
mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp | ||
---|---|---|
2452 | I think it's a matter of Stanrard -> LLVM conversion not being able to handle non-identity affine maps. The Op itself is able to represent them without issues. This is true with other memref types that are handled in Standard -> LLVM, they all don't deal with non-identity affine maps. So it seems the check belongs here. | |
2516 | I think that might be better than a nullptr because free(nullptr) is a nop. Let me do that. |
mlir/test/mlir-cpu-runner/global_memref.mlir | ||
---|---|---|
38 | I haven't seen any other tests using CHECK-LABEL in these tests. I'd assume for CHECK-LABEL, we would need to dump out something like a label or header from these tests with a printString() like function if available. However, I don't see such function. One option is to introduce a new function either to print a string or print a header (taking in an int64 id). Standard -> LLVM does not seem to support string type constants (yet). Are we ok with revisiting headers (in not just thins but all other mlir-cpu-runner tests) once we have support for this? |
This may be related to failure in https://buildkite.com/mlir/mlir-core/builds/9228#25f0ef1a-60e1-4f0e-a231-417f3747a366
mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp | ||
---|---|---|
2436–2438 | Nit: type conversion can fail |
mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp | ||
---|---|---|
2436–2438 | I didn't see a similar check in other places like this. Let me add it here though and fail the conversion when that happens (in a new review). |
Nit: type conversion can fail