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

APInt dialect draft #2242

Closed
wants to merge 15 commits into from
Closed

APInt dialect draft #2242

wants to merge 15 commits into from

Conversation

7FM
Copy link
Contributor

@7FM 7FM commented Nov 26, 2021

This PR shows our first draft of a new dialect for sign-aware lossless arbitrary precision integer arithmetics.
This is not intended as PR but rather as RFC :)

Ping @jopperm

@jopperm
Copy link
Contributor

jopperm commented Nov 26, 2021

Thanks (and welcome) @7FM!

Some context: In the last Calyx-Scheduling call, it came up that a dialect to model APInt (and later, APFixed) semantics, inspired by what Xilinx offers in form of a C++ template library for Vitis HLS, would be useful to have in CIRCT. My group at TU Darmstadt was just about to built such a thing internally, so I suggested that we could move development here instead.

The rationale for building such a thing is, to my mind:

  • Ability to capture user-defined APInt computations (e.g. coming from a library or DSL) without making the zero/sign-extensions explicit early in the flow.
  • Simpler bitwidth optimizations, for the same reason (no zext/sext). Maybe this could even be implemented locally via pattern matching, instead of needing a DFA.

In the end, the operations of this dialect would lower seamlessly to comb and signless integers.

As @7FM writes, this is a very early and incomplete draft to a) announce we are working on this and b) gather feedback on the mechanics of implementing the respective type rules (adding more ops should be mechanical).

Curious to hear what you all think!

@mikeurbach
Copy link
Contributor

Thanks for the draft. While I always appreciate having some code to discuss, this may get a little more visibility and discussion as an RFC on Discourse: https://llvm.discourse.group/c/projects-that-want-to-become-official-llvm-projects/circt/40.

Regardless, I think having a dialect for arbitrary precision arithmetic seems like a good idea. A few thoughts I had:

  • The current proposal defines new operations in terms of the builtin IntegerType. That seems like a good place to start, but I'm curious if you've considered defining your own types. It may be overkill if all you care about is bitwidth and signedness.
  • I'm curious if @stephenneuendorffer has any feedback on taking the same approach as Vivado HLS. Was this successfull or were there things that might be worth reconsidering.
  • Would this grow to encompass fixed point arithmetic, or would that be a separate dialect? I think we have said before that a CIRCT "math" dialect might be appropriate, so perhaps this dialect could be the start of that, and home to both integer and fixed point arithmetic. Pragmatically speaking, it just comes down to naming for now, but I think it's important to discuss the goals and have a small "charter" for a new dialect.

Copy link
Collaborator

@lattner lattner left a comment

Choose a reason for hiding this comment

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

I have a few high level concerns about this:

  1. Please don't name this "APInt". LLVM already has an APInt class which is used pervasively throughout CIRCT and this will be confusing, please pick another name.

  2. Please don't define the semantics of the operations by reference to an external spec, please specify what they are. If this is intended to match Vivado's behavior, then this should get a Vivado specific dialect name. It shouldn't pretend to be something more general.

  3. The inference rules don't seem to handle signless types explicitly, these should be rejected by the verifier, right?

  4. This needs significantly more design discussion (e.g. in a weekly design meeting) so we can understand how it will be used and how it fits into the system. For example, why isn't this in upstream MLIR? There is nothing about it that appears to be HW specific.

lib/Dialect/APInt/APIntTypes.cpp Outdated Show resolved Hide resolved
@7FM
Copy link
Contributor Author

7FM commented Nov 27, 2021

  1. The inference rules don't seem to handle signless types explicitly, these should be rejected by the verifier, right?

Yes, my intention is currently to prohibit signless types by using the APIntType.

@7FM
Copy link
Contributor Author

7FM commented Nov 27, 2021

  • The current proposal defines new operations in terms of the builtin IntegerType. That seems like a good place to start, but I'm curious if you've considered defining your own types. It may be overkill if all you care about is bitwidth and signedness.

To be honest, I am completely new to MLIR and CIRCT related projects and therefore quite happy to find suitable builtins and have not considered own types yet.
And as you mentioned, currently it is probably overkill. However, if this might grow to encompass i.e. fixed point arithmetic then custom types might be needed. But I also haven't checked for other builtins that might support this.

@jopperm
Copy link
Contributor

jopperm commented Nov 28, 2021

Thank you both for taking the time to have a look at this; your comments give us a very good sense of what we need to address before we can discuss this properly in a future ODM or Discourse RFC!

Please don't define the semantics of the operations by reference to an external spec, please specify what they are. If this is intended to match Vivado's behavior, then this should get a Vivado specific dialect name. It shouldn't pretend to be something more general.

Agreed, but please allow me to clarify: This PR is about operations that take different operand types and produce results without loss of precision and sign information in a two's complement representation. For example, adding a ui3 and an si3 will yield an si5 value.

So if one kind of commonly used arithmetic semantics is the arith/comb-style arithmetic with the same operand and results types, I'd argue the one here is the "other". The proposed dialect is not Vivado-specific, it just so happens that Xilinx' docs provide a well-thought-out specification of the type rules.

@stephenneuendorffer
Copy link
Contributor

stephenneuendorffer commented Nov 29, 2021

Hi all! Great to see you're working on this. I've been thinking this would be a good step forward for a while.
Generally, I think this approach will have some advantage for the compiler process. I've definitely found that it's a useful programming model for HLS systems. The main point of complexity is the result bitwidths of left shifts. For this I would just not worry about representing shifts at all and just use arith.shift.

Note that for fixed point, result bitwidth for division is also problematic. My suggestion would be follow what Vivado HLS does for division (basically following a rule that if the result can be exactly represented, then the result has that number of bits).

The current proposal defines new operations in terms of the builtin IntegerType.

I like this...

Please don't name this "APInt". LLVM already has an APInt class which is used pervasively throughout CIRCT and this will be confusing, please pick another name.

I'm somewhat ambivalent here. If we try to avoid all possible namesquatting then we're left with things that are so generic, they don't mean much. (For instance, the "hw" dialect comes to mind.) Vivado HLS uses "ap_int". "mp" or "mp_int" for multi-precision might be another option. Ultimately, we definitely need to document what this is unambiguously for users.

Please don't define the semantics of the operations by reference to an external spec, please specify what they are. If this is intended to match Vivado's behavior, then this should get a Vivado specific dialect name. It shouldn't pretend to be something more general.

The catapult C "ac_int" type is another example on these lines that is very similar.

This needs significantly more design discussion (e.g. in a weekly design meeting) so we can understand how it will be used and how it fits into the system. For example, why isn't this in upstream MLIR? There is nothing about it that appears to be HW specific.

My perspective on this is that CIRCT is a good place to incubate this. If we want to move it upstream, then there's nothing to prevent us from doing that. (In particular, I think it might be useful as a complement to the 'arith' dialect') I think we can cross that bridge when we come to it. I think having some transformations and lowerings would be a good thing. Note that from a user's perspective part of the value of this model is that a user can often least widths unspecified and rely on width inference to compute the right width. This advantage may not be present in a compiler IR where all the width have to match up. In particular, I think we should at least implement a lowering into arith and a bitwidth inference pass on this dialect.

let summary = "Types and operations for apint dialect";
let description = [{
This dialect defines the `apint` dialect, which is intended to represent
lossless arbitrary precision integer arithmetics.
Copy link
Contributor

Choose a reason for hiding this comment

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

A little more verbosity would be useful here. In what sense are the operations "lossless"? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will try my best :)

lib/Dialect/APInt/APIntOps.cpp Show resolved Hide resolved
@lattner
Copy link
Collaborator

lattner commented Nov 30, 2021

To be honest, I am completely new to MLIR and CIRCT related projects and therefore quite happy to find suitable builtins and have not considered own types yet.
And as you mentioned, currently it is probably overkill. However, if this might grow to encompass i.e. fixed point arithmetic then custom types might be needed. But I also haven't checked for other builtins that might support this.

Awesome and welcome!

Beyond technical considerations and in addition to being conceptually useful, we need to make sure that there are clients for this code, that it gets tested in a flow, and that it will be maintained over time, etc. Once we understand the use-case, we can figure out what the right technical approach is.

I'd recommend that we discuss this as part of the open design meeting (e.g. tomorrow). Are you available to discuss there?

-Chris

@7FM
Copy link
Contributor Author

7FM commented Dec 1, 2021

Awesome and welcome!

Thanks :)

Beyond technical considerations and in addition to being conceptually useful, we need to make sure that there are clients for this code, that it gets tested in a flow, and that it will be maintained over time, etc. Once we understand the use-case, we can figure out what the right technical approach is.

I'd recommend that we discuss this as part of the open design meeting (e.g. tomorrow). Are you available to discuss there?

Tomorrow (1.12.) is a bit short notice, but we (@jopperm and I) will be happy to participate on one of the next ODMs (e.g. 8.12. or 15.12)!

@lattner
Copy link
Collaborator

lattner commented Dec 1, 2021

Sounds great!

@7FM
Copy link
Contributor Author

7FM commented Dec 14, 2021

Thanks for all the feedback! For now we will continue developing our JulianDialect and will see if something comes up to contribute here.

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.

None yet

5 participants