This is an archive of the discontinued LLVM Phabricator instance.

[MLIR] Support `global_memref` and `get_global_memref` in standard -> LLVM conversion.
ClosedPublic

Authored by jurahul on Nov 4 2020, 3:52 PM.

Details

Summary
  • 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.

Diff Detail

Event Timeline

jurahul created this revision.Nov 4 2020, 3:52 PM
Herald added a project: Restricted Project. · View Herald Transcript
jurahul requested review of this revision.Nov 4 2020, 3:52 PM
silvas accepted this revision.Nov 4 2020, 4:10 PM

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"

This revision is now accepted and ready to land.Nov 4 2020, 4:10 PM
jurahul updated this revision to Diff 302994.Nov 4 2020, 5:06 PM
jurahul marked an inline comment as done.

Address comments

jurahul marked an inline comment as done.Nov 4 2020, 5:09 PM
jurahul added inline comments.
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.

@mehdi_amini or @ftynse, can one you take a look as well?

mehdi_amini accepted this revision.Nov 6 2020, 4:39 PM
mehdi_amini added inline comments.
mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp
2452

Shouldn't this be handled in the verifier for the op?

2516

I've been wondering if some magic value like 0xDEADBEEF could help debugging as well potentially.

jurahul added inline comments.Nov 6 2020, 5:02 PM
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.

bondhugula added inline comments.
mlir/test/mlir-cpu-runner/global_memref.mlir
37

Please use a CHECK-LABEL to avoid overrun matches.

53

Please use a CHECK-LABEL to avoid overrun matches.

85

Likewise.

jurahul marked 2 inline comments as done.Nov 9 2020, 8:53 AM
jurahul added inline comments.
mlir/test/mlir-cpu-runner/global_memref.mlir
37

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?

jurahul updated this revision to Diff 303896.Nov 9 2020, 9:02 AM

Use 0xdeadbeef for allocated pointer

ftynse added inline comments.Nov 9 2020, 12:16 PM
mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp
2436–2438

Nit: type conversion can fail

jurahul marked 4 inline comments as done.Nov 9 2020, 1:05 PM
jurahul added inline comments.
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).