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

TIR: Intrinsic overhaul, remove hash-intrinsics and make everything more declarative #969

Merged
merged 11 commits into from
Sep 13, 2023

Conversation

kontheocharis
Copy link
Collaborator

@kontheocharis kontheocharis commented Sep 12, 2023

  • Both intrinsics and primitives are now declared in hash_tir::intrinsics::definitions in a declarative syntax that mirrors Hash.
  • The macro which accepts this DSL generates various convenient items for using and referencing these definitions.
  • To represent intrinsics in the TIR, a new term Term::Intrinsic is added, which accepts a single enum argument that represents the chosen intrinsic.
  • There is no longer dynamic dispatch for calling intrinsics, which should improve compile-time performance.
  • Intrinsics are now allowed to return Ok(None) to signal that the call to the intrinsic cannot be simplified.

Closes #963

@feds01 I have left a @@Todo in the function discoverer for lowering, I am not sure if I should discover the intrinsics as well or that isn't needed anymore.

@kontheocharis kontheocharis added the type-system Issues related with typechecking sub-system. label Sep 12, 2023
@kontheocharis kontheocharis self-assigned this Sep 12, 2023
@kontheocharis kontheocharis changed the title TIR: Intrinsic overhaul, removing hash-intrinsics and making everything more declarative TIR: Intrinsic overhaul, remove hash-intrinsics and make everything more declarative Sep 12, 2023
Cargo.toml Show resolved Hide resolved
Comment on lines +18 to +21
use hash_tir::intrinsics::definitions::{
BinOp as TirBinOp, CondBinOp as TirCondBinOp,
ShortCircuitingBoolOp as TirShortCircuitingBoolOp, UnOp as TirUnOp,
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Haha, verbose naming 💯

I feel like we should unify operators in hash-source, especially since we will kind of need to do that if we want to share using the Const representation across the compiler. Perhaps, even define it in hash-token?

It would eliminate:

  • ir::BinOp ir::UnOp
  • ast::BinOp ast::UnOp
  • tir::BinOp tir::UnOp

But maybe the separation makes sense?

Copy link
Collaborator Author

@kontheocharis kontheocharis Sep 12, 2023

Choose a reason for hiding this comment

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

I think it's a bit different because the operators defined in the AST do not necessarily correlate with the "intrinsic" operations that can be defined on TIR terms. For example, with the new constant stuff a lot of the intrinsics will greatly simplify/become redundant. I think I will first adapt to Const in another PR and then we can consider this separation more.

Copy link
Contributor

Choose a reason for hiding this comment

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

When we do integrate the new constant stuff, we will still have to define some kind of common operator kinds, I guess for now the IR can just rely on them and the TIR can define conversions for them.

Copy link
Collaborator Author

@kontheocharis kontheocharis Sep 13, 2023

Choose a reason for hiding this comment

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

Okay. I agree that there should be some unification here.. Yet another todo item

compiler/hash-tir/src/intrinsics/utils.rs Outdated Show resolved Hide resolved
compiler/hash-tir/src/intrinsics/mod.rs Outdated Show resolved Hide resolved
compiler/hash-lower/src/build/ty.rs Outdated Show resolved Hide resolved
compiler/hash-lower/src/discover.rs Outdated Show resolved Hide resolved
Co-authored-by: Alexander Fedotov <alexander.fedotov.uk@gmail.com>
@kontheocharis kontheocharis merged commit c4f7d80 into main Sep 13, 2023
1 check passed
@kontheocharis kontheocharis deleted the intrinsic-overhaul branch September 13, 2023 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-system Issues related with typechecking sub-system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Intrinsics should be in an enum, same as primitives
2 participants