Skip to content
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

Bump LLVM to 10407be542aeb2b59477b167bbba3716538dc722. #7550

Merged
merged 8 commits into from
Sep 1, 2024

Conversation

mikeurbach
Copy link
Contributor

Standard LLVM version update.

@mikeurbach
Copy link
Contributor Author

Opening this now, because I want to get some eyes on it. This is mostly a benign bump, but one thing has merged upstream that affects SystemC: llvm/llvm-project#91475. More discussion here: https://discourse.llvm.org/t/psa-modelling-memory-of-emitc-variables/80556.

Concretely, the only thing I saw that effects our tests is this:

%1 = emitc.apply "&"(%five) : (!emitc.opaque<"int">) -> !emitc.ptr<!emitc.opaque<"int">>

emitc.apply "&" now requires an lvalue type for its operand.

I'm curious what @maerhart thinks... it's been a while since I looked at the SystemC dialect, but it looks like we have our own ops for modeling lvalues (systemc.cpp.variable, systemc.cpp.assign, etc.). I'm not sure how far we want to go to down the road of compatibility with EmitC dialect's lvalues. Perhaps systemc.cpp.variable's result type could be an lvalue type, and the destination of systemc.cpp.assign could be an lvalue type. Or we could start using the upstream EmitC ops instead. I took a brief look at the former, but decided it would be best to let the experts weigh in so I've sent this PR as-is. The in-tree CI should pass, but I'm not sure if there are deeper things to consider.

FYI I will now be on vacation for a week, so please feel free to pick this up while I'm out or chat with @dtzSiFive who has offered to help with LLVM bumps.

@maerhart
Copy link
Member

Thanks @mikeurbach
I think making the result of systemc.cpp.variable an lvalue is a good solution.
We might want to revisit the systemc dialect at some point because a lot has changed over the last year in the upstream emitc dialect and we can probably reuse a lot more of their ops, but that's a topic for another PR. E.g., the variable op upstream is still slightly different in that it only accepts an attribute as the init value, so replacing it might be a bit tricky.

I can take care of this on Friday if nobody else has picked it up until then.

@dtzSiFive
Copy link
Contributor

I can take care of this on Friday if nobody else has picked it up until then.

That would be great, thank you!

@maerhart
Copy link
Member

After taking a closer look at the current state of EmitC, I noticed that EmitC in its current state is quite locked down and doesn't like to be mixed with other dialects. For example, the lvalue type verifies that the nested type is an emitc type and many operations do the same. This means, it is not possible to have a custom type representing some C struct (or even C++ class) explicitly rather than via the opaque type and directly use it with the emitc operations and types.
One thing that complicates the situation is that SystemC doesn't use pure C but also C++ constructs.
I see these paths forward for SystemC:

  1. Don't reuse any emitc operations to have full control over our stack: requires a lot of operations that are mostly just duplicates of emitc versions but adapted to our type system
  2. Allow mixing them but only via a conversion op that converts between SystemC and EmitC types: SystemC types can be cast to EmitC opaque types, possibly using a type interface that each SystemC type can implement to verify the conversion operations. SystemC will not use emitc types directly in its operations (except the conversion op) anymore.
  3. Modify the EmitC dialect to be more flexible and allow external types in their operations and nested types as long as they implement a (new) interface that lets EmitC know how to emit them or convert them to opaque versions. Not sure if that's in scope for the EmitC dialect.

I think option 2 is the most promising one because it keeps both the SystemC and EmitC dialects simple and separate. Given the frequent upstream changes this will help with downstream integration. It also leads to a clear boundary between the SystemC/C++ and the C parts.

Let me know what you think.

To unblock this bump, I'll just untangle the tests a bit such that the systemc and emitc parts are more separated.

@maerhart maerhart marked this pull request as ready for review August 30, 2024 19:19
@maerhart maerhart requested a review from uenoku August 30, 2024 19:20
Copy link
Member

@uenoku uenoku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you for updating initial op lowering. Interesting that unrealized_conversion cast is removed even whey cast is not introduced by dialect conversion.

@maerhart maerhart merged commit c2c0473 into main Sep 1, 2024
4 checks passed
@maerhart maerhart deleted the mikeurbach/bump-llvm branch September 1, 2024 08:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants