[RFC] Switching the LLVM Dialect and Dialect lowerings to opaque pointers

MLIR
1

As of the version bump to LLVM 17, typed pointers within LLVM are not supported anymore [0] and will be removed as soon as possible.
To mirror these changes I’d like to propose changing the LLVM Dialect in MLIR to only support opaque pointers, which will allow a major cleanup of the LLVM Dialect and having it follow suite of LLVM IR.

The difficulty in doing this switch is in migration paths for downstream projects and the fact that MLIR essentially has many frontends (all the dialect lowerings!) which have to be switched.
Since there are some very important details in rewriting these lowerings I decided to write this RFC.

Steps required

The gist of changes required boil down to basically:

  • add opaque pointer support to upstream conversion passes
  • wait for downstream to switch their lowerings to opaque pointers and to make use of upstream conversion passes
  • remove typed pointers from the LLVM dialect entirely

Modulo bugs, the LLVM Dialect already supports opaque pointers.

Conversion passes

The current issue, and why not many downstream projects have probably switched to opaque pointers yet is that as of writing this, upstream conversion passes (e.g. -finalize-memref-to-llvm) do not yet support lowering to opaque pointers.
Using opaque pointers as opposed to typed pointers is currently an all or nothing setting. Either the whole IR has to use opaque pointers or typed pointers.

I therefore propose the following:
All conversion passes making use of pointer types get a new pass option called use-opaque-pointers, defaulting to 0, which when enabled, will make the conversion pass emit LLVM Dialect IR using opaque pointers. LowerToLLVMOptions also gets a new option useOpaquePointers, which acts as a common interface for conversion patterns (and the LLVMTypeConverter), to know whether it should emit opaque pointers.
In my current implementation LLVMTypeConveter also offers a getter checking whether opaque pointers should be use, as well as the convenience method getPointer, which will return a correct pointer type that is either opaque or not given the current setting.

Rationale:
I believe there has to be some amount of time that the conversion passes support both typed and opaque pointers. Downstream projects currently cannot do the switch, since the upstream conversions need to adopt opaque-pointers but at the same time, switching conversion passes to only allow opaque pointers would immediately break downstream projects.

Tests of conversion passes

This is the point I am most conflicted about and where I’d like to hear more opinions. The gist of the issue is how to both support opaque pointers and typed pointers.

I have currently thought of following ways we could handle the testsuite:

  • Radical approach: Switch the testsuite of a pass to opaque pointers, don’t bother testing typed pointers
    Pros: Follows LLVM IR; Makes divergence of typed and opaque pointer tests impossible; Less changes
    Cons: Loss of test coverage for typed pointers and generally unclear what to do about bugs found in typed pointer specific code; Prone to regress typed pointer support while it may still be needed.
  • Duplicate: Copy any tests with pointers, rename the file to add the suffix -typed-pointers, and rewrite the main file to use opaque pointers.
    Pros: Coverage of typed pointer support remains as is.
    Cons: Possibility of the test files for typed pointers and opaque pointers to diverge and the need to test both for new lowerings being added, risking regression in the support of either otherwise.

One could also possibly adopt some more nuanced strategy than either of the above, but we will need some kind of policy.

Implementation Status

As a first step I have already implemented the changes proposed above with the “Duplicate” testsuite for the -finalize-memref-to-llvm conversion pass, allowing MemRefs to be lowered to LLVM using opaque pointers. It also adds the initial infrastructure in LLVMCommon for further conversion passes to be adapted.
I have written this RFC following the experience from writing that change.
You can find the review here to get a feeling for the changes required:
https://reviews.llvm.org/D143268

I believe that this is the conversion passes requiring the most amount of changes than any other dialects, so it should not get any worse than this.

[0] Opaque Pointers — LLVM 17.0.0git documentation

6 Likes
2

Thanks for undertaking this! The proposed approach makes sense to me.

I added opaque pointer support to most (but not all) LLVM dialect operations a while back, with the goal of preserving the syntax as much as possible (i.e., the trailing !llvm.ptr<i64> in a load becomes !llvm.ptr -> i64) and that could be the way to follow to minimally disrupt the existing tests. At Google, we have plenty of downstream tests that FileCheck LLVM dialect, and it seems to be the case in some other projects, so minimizing syntactic churn and providing a graceful transition period is highly appreciated.

The flag-based approach sounds good. If possible, I would encourage some sort of timeline on how the things will happen. A relative one should do, e.g., 2 weeks after all upstream passes are updated, the flag switches to default-on and is declared deprecated, 2 weeks after that and in absence of major regressions the flag is removed and the typed pointer support is dropped completely.

Regarding the tests, I’d go for a hybrid variant as follows:

  • tests for passes that support opaque pointers are switched to use opaque pointers everywhere;
  • some “basic” tests for typed pointers are duplicated, e.g., one “happy path” test per lowering pattern, but no exhaustive testing;
  • reporters of bugs regarding typed pointers are first encouraged to switch to opaque pointers as a fix, and if that doesn’t help, a test for both typed and opaque pointers exercising the buggy behavior is introduced.

This would keep most of the coverage while reducing duplication.

3

Thanks a lot for working on the opaque pointer switch!

We are currently able to import and export some (although still relatively trivial) C++ / LLVM IR using opaque pointers only. I would thus hope there are no big surprises for the common operations.

4

Thank you for your response!

Your suggestion for how to handle test existing testsuites was the nuanced opinion I needed :slight_smile: I will try to apply it to my review.

Regarding the timeline I fear I may have not have the best feeling for how much work is required downstream. I believe a lot of users of MLIR are making use of the upstream conversion passes, in which case it’d simply be a matter of switching the flag (or having the new default apply), for them to switch to an opaque pointer representation.
For any other custom lowerings the work required is okayish in difficulty, but a lot of manual work nevertheless since one has to go over every GEP, load and alloca instruction.

Your proposed timeline would give a month in total for downstream users to change before being cut-off completetly. I think that is probably the bare minimum of time we should give people.
If too short I’d suggest doubling the time, so switching the default flag after upstream passes have changed in a month, another another month without regressions after that until typed pointer support is removed completetly.

Would love feedback from any downstream users in that regard!
Speaking from my experience of switching a downstream project to opaque pointers it only took roughly 2 days of work. Similarily the initial patch I posted here took about the same amount of work.

5

Thanks @zero9178 for working on this and for creating the patch ⚙ D143582 [mlir][OpenMP] Add support for using Opaque Pointers in the OpenMP Dialect to add opaque pointers from the OpenMP Dialect. The patch looks fine. But I have one question regarding the general direction MLIR and the dialects are going. Will high level dialects like memref also drop the elementType from it and switch to being opaque? The reason I am asking is because we have this OpenMP_PointerLikeTypeInterface that currently demands a getElementType. It has been a useful Interface to restrict Operands where we expect variables. But with LLVM dialect switching to Opaque types, getElementType can potentially return NULL making the interface not that useful at the LLVM level but continuing to work for other types like memref and FIR.

6

I cannot speak for the owners of (or the community around) these dialects, but I very much doubt the element type will disappear from types such as memref. Memrefs, LLVM and other dialects simply work on different abstraction levels and the fact that element types in LLVM IR were more problematic does not imply that being the case for any higher level dialect. What works on LLVMs abstraction level does not have to work well on a higher abstraction level.

The patch as is will continue to do this verification for memref and FIR. The fact that it is now less strict when using opaque LLVM pointer types is a bit unfortunate, but fits the semantics of LLVM (given that memory in LLVM has no type).

3 Likes
7

Thanks @zero9178. I will come back to this tomorrow. Hope that is OK. I would also like to check whether @ftynse has an opinion on this.

1 Like
8

I agree with @zero9178. I don’t think memref will remove the elemental type: without the type, the sizes don’t really make sense either because the addressing scheme needs to account for the size and layout of a type. Put otherwise, given an untyped memref, it isn’t helpful to know it points to 42 elements of something without also knowing the size/layout of that something. And without the number of elements, memref decays to being just an opaque pointer.

9

Hello! Sorry for a small offtopic :slight_smile:

I’ve just faced with a bug related to that “opaque pointers” switch activity. It looks like createConvertFuncToLLVMPass(const LowerToLLVMOptions &options) doesn’t honor options.useOpaquePointers. Instead it internally creates separate LowerToLLVMOptions object and initialize its useOpaquePointers field from the pass CMD option (which is false by default).

I’m just curios is this a bug? Why createConvertFuncToLLVMPass(const LowerToLLVMOptions &options) doesn’t use the provided LowerToLLVMOptions as is instead of creating internal LowerToLLVMOptions object?

10

@zero9178 is away from keyboard.

I hope this ⚙ D143733 [mlir][func] Use the generated pass options in func to llvm. fixes the problem?

The idea is to use the tablegen generated pass options struct that should always stay in sync with the actual pass options. This seems to be in line with the recent pass infra refactoring [PSA] (Deprecation warning) Per-pass autogenerated macros and pass options.

2 Likes