-
Notifications
You must be signed in to change notification settings - Fork 454
[1/2] [MooreToCore] Lower symbolic vtables to LLVM globals #10129
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Draft
Scheremo
wants to merge
1
commit into
llvm:main
Choose a base branch
from
Scheremo:pr-class-vtable-3
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+259
−8
Draft
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This conversion from
func.functollvm.funcfeels a bit weird, because it only converts a handful of functions that happen to be involved in a class vtable. What happens if the function is not a function but a task, or if it contains LLHD and other ops that interact with an event queue? The fact that the LLVM forces us to unravel most of the IR at this early stage is a bit of a red flag -- we might need another dialect layer in between here. (At the CIRCT core dialects we're still dealing with hardware concerns and higher-level simulation semantics; LLVM might be too low level for some of that 🤔.)Does the
llvm.mlir.addressofop enforce that the target is a global variable or a function? If it does, we might want to try and defer lowering to LLVM until a later point, and see if we can't build up vtables with other MLIR dialects instead of LLVM. Maybe thefunc.constantop could help create SSA values with a function type, which we could store in a struct and later call withfunc.call_indirect? We'll probably also need an equivalent for tasks/coroutines.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really good point about the tasks! Hadn't really thought about that yet 🤔
I think the "natural" representation of a task at this level would be a coroutine - in that way there would also be a LLVM structure to represent it.
I had tinkered with staying with
func.funcfor as long as possible but ran into issues with materialising the vtable since I didn't find a way to properly indirect function calls withcall.indirect(Hadn't had a look atfunc.constant, though🤦)From my point of view this approach also doesn't really have to unravel any IR except for the direct lowering of
func.functollvm.func(mainly so I can get a function pointer to save into a dispatch table). Thellvm.funcbody contains "the same things" afunc.funcwould. In that sense it's definitely worth trying withfunc.constant, though - would feel like a nicer solution 👍There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🥳 My suspicion/fear is that once the outer op is an
llvm.func, we'll start inheriting constraints from it -- maybe certain terminators no longer work, yield doesn't work (LLVM coroutines aren't really what we want in CIRCT), and things likellvm.callmight not work in all contexts. LLVM feels a lot like an egress dialect, which really wants an all-LLVM-or-nothing setup. If we run into trouble withfunc.call_indirector the vtables, we could consider extending the Sim dialect with a corresponding construct that then has a trivial lowering to the LLVM you're generating here... WDYT?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if I manage to tickle those
func.constantoperators juuuust the right way and potentially materialise vtables a bit later (i.e between MooreToCore and convert-to-llvm) we might just get "de Foifer und 's weggli" with this one 👀We might then consider legalising
moore.vtable.*for core dialects, so we have a structure to record table entry order. But I guess that's a very small price to pay and can be worked around.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing hits quite like de Foifer und 's weggli! 😏 5️⃣ 🥖
About that
moore.vtable.*legalization: one of the things you could do is place an op in between the LLVM lowering you were targeting in this PR, and the originalmoore.vtable.*ops. Something likesim.vtablethat is just the flat, distilled-down list of functions that can be called, addressed by index. Something that has an almost trivial 1:1 lowering to LLVM. That allows you to hand-off the Moore-level abstraction into something that's closer to the target while we're at the core dialect level, and then there's a final mechanical switch in convert-to-llvm.