This is an archive of the discontinued LLVM Phabricator instance.

[mlir][MemRef] Add option to `-finalize-memref-to-llvm` to emit opaque pointers
ClosedPublic

Authored by zero9178 on Feb 3 2023, 7:07 AM.

Details

Summary

This is the first patch in a series of patches part of this RFC: https://discourse.llvm.org/t/rfc-switching-the-llvm-dialect-and-dialect-lowerings-to-opaque-pointers/68179

This patch adds the ability to lower the memref dialect to the LLVM Dialect with the use of opaque pointers instead of typed pointers. The latter are being phased out of LLVM and this patch is part of an effort to phase them out of MLIR as well. To do this, we'll need to support both typed and opaque pointers in lowering passes, to allow downstream projects to change without breakage.

The gist of changes required to change a conversion pass are:

  • Change any LLVM::LLVMPointerType::get calls to NOT use an element type if opaque pointers are to be used.
  • Use the build method of llvm.load with the explicit result type. Since the pointer does not have an element type anymore it has to be specified explicitly.
  • Use the build method of llvm.getelementptr with the explicit basePtrType. Ditto to above, we have to now specify what the element type is so that GEP can do its indexing calculations
  • Use the build method of llvm.alloca with the explicit elementType. Ditto to the above, alloca needs to know how many bytes to allocate through the element type.
  • Get rid of any llvm.bitcasts
  • Adapt the tests to the above. Note that llvm.store changes syntax as well when using opaque pointers

I'd like to note that the 3 build method changes work for both opaque and typed pointers, so unconditionally using the explicit element type form is always correct.

For the testsuite a practical approach suggested by @ftynse was taken: I created a separate test file for testing the typed pointer lowering of Ops. This mostly comes down to checking that bitcasts have been created at the appropiate places, since these are required for typed pointer support.

Diff Detail

Event Timeline

zero9178 created this revision.Feb 3 2023, 7:07 AM
zero9178 requested review of this revision.Feb 3 2023, 7:07 AM
Herald added a project: Restricted Project. · View Herald Transcript
zero9178 edited the summary of this revision. (Show Details)Feb 3 2023, 7:07 AM
zero9178 retitled this revision from [mlir][MemRef] Add option for `-finalize-memref-to-llvm` to emit opaque pointers to [mlir][MemRef] Add option to `-finalize-memref-to-llvm` to emit opaque pointers.
zero9178 updated this revision to Diff 494625.Feb 3 2023, 7:11 AM

Rebase in hopes of pre-merge CI being able to apply it

zero9178 updated this revision to Diff 494908.Feb 5 2023, 6:55 AM
  • rebased tests
  • split required address space into a separate patch that this patch now depends on
  • changed testing strategy of typed pointers as discussed in the RFC:

My strategy was to look at the tests prior to conversion to opaque pointers and copy those over affected by any of the new code paths. This mostly boils down to requiring bitcasts in a few places. Changes to GEPs, loads, allocs and stores are in unconditional code paths and the correct application of element types also tested by the opaque pointer tests.

gysit added a comment.Feb 6 2023, 1:00 AM

Thanks for separating the address space conversion issue into a separate revision. I have some minor nit comments mostly around using getPointer in more places. Feel free to ignore them if there was a reason for not using it.

Overall LGTM!

The next step is probably fixing the CI issue and give others a chance to have a look at the revision as well.

mlir/include/mlir/Conversion/LLVMCommon/TypeConverter.h
131

ultra nit: getPointer -> getPointerType?

mlir/lib/Conversion/LLVMCommon/Pattern.cpp
51–52

nit: Would getTypeConverter()->getPointer(IntegerType::get(&getTypeConverter()->getContext(), 8)) work here as well?

114–116

nit: Could this also be a use case for getPointer()?

mlir/lib/Conversion/LLVMCommon/TypeConverter.cpp
206–207

nit: Using getPointer() below may also be an option?

mlir/test/Conversion/MemRefToLLVM/convert-dynamic-memref-ops.mlir
260–261

Would it make sense to delete them from the opaque pointer tests since they moved into the typed pointer test?

zero9178 updated this revision to Diff 495055.Feb 6 2023, 2:53 AM
zero9178 marked 4 inline comments as done.

Address review comments:

  • Rename getPointer to getPointerType (which I agree is a more fitting name)
  • Make use of getPointerType in a few more places I have previously missed
  • Remove tests that became noops in opaque pointer mode (they're still present in the typed-pointers tests)
  • clang-format files in hopes of fixing CI
zero9178 marked an inline comment as done.Feb 6 2023, 2:54 AM
zero9178 updated this revision to Diff 495062.Feb 6 2023, 3:11 AM

clang-format again

zero9178 edited the summary of this revision. (Show Details)Feb 6 2023, 4:11 AM
gysit accepted this revision.Feb 7 2023, 6:18 AM

Thanks for the changes LGTM!

This revision is now accepted and ready to land.Feb 7 2023, 6:18 AM