- 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.
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.
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.
I find this name a bit confusing. maybe "needs buffer size calculation"?
nit "lvm" -> "llvm"
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).
I renamed the function as well as the cumulativeSize variables to just bufferSize.
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.
I think that might be better than a nullptr because free(nullptr) is a nop. Let me do that.
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?
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).