feat: Add emitters for int <-> float/usize conversions #94

croyzor · 2024-09-03T13:58:12Z

Work towards #22, but since we updated to hugr 0.12.0 there are more operations in arithmetic.conversions. I'll implement those in a separate PR.

src/emit/ops.rs

codecov-commenter · 2024-09-03T14:01:50Z

Codecov Report

Attention: Patch coverage is 84.43709% with 47 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@aba5bd0). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
src/custom/conversions.rs	88.88%	18 Missing and 9 partials ⚠️
src/custom/int.rs	56.25%	12 Missing and 2 partials ⚠️
src/emit/func.rs	0.00%	3 Missing ⚠️
src/emit/ops.rs	50.00%	0 Missing and 2 partials ⚠️
src/custom/float.rs	95.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main      #94   +/-   ##
=======================================
  Coverage        ?   82.26%           
=======================================
  Files           ?       20           
  Lines           ?     2972           
  Branches        ?     2972           
=======================================
  Hits            ?     2445           
  Misses          ?      333           
  Partials        ?      194

Flag	Coverage Δ
rust	`82.26% <84.43%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/custom/conversions.rs

src/emit/ops.rs

src/custom/conversions.rs

Co-authored-by: Mark Koch <[email protected]>

zext does what i thought sext did!

acl-cqc · 2024-09-10T15:28:36Z

src/custom/conversions.rs

+
+        match conversion_op.def() {
+            ConvertOpDef::trunc_u => {
+                // This op should have one type arg only: the log-width of the


How about puttig this comment in the message from the panic?

acl-cqc · 2024-09-10T15:30:28Z

src/custom/conversions.rs

+                self.build_trunc_op(false, log_width, args)
+            }
+
+            ConvertOpDef::trunc_s => {


You could do

ConvertOpDef::trunc_u | ConvertOpDef::trunc_s => { let signed = conversion_op.def() == ConvertOpDef::trunc_s; ... self.build_trunc_op(signed, log_width, args) }

acl-cqc · 2024-09-10T15:44:04Z

src/test.rs

-    /// Lower `hugr` to LLVM, then JIT and execute the function named `entry` in
-    /// the inner module.
+    /// Lower `hugr` to LLVM, then JIT and execute the function named
+    /// `entry_point` in the inner module.


Suggested change

/// `entry_point` in the inner module.

/// by `entry_point` in the inner module.

I'm not quite sure what the "inner" "module" is (if AIUI we have a Hugr, not a Package, isn't that a/the-only module?), but nvm

acl-cqc · 2024-09-10T15:44:53Z

src/test.rs

    ///
-    /// That function must take no arguments and return an `i64`.
+    /// That function must take no arguments and return an LLVM `i64`.


Seems odd that the function must return an i64 when this method is called exec_hugr_u64...

exec_hugr_u64 refers to the hugr u64 that's returned, which is converted to an LLVM i64 (LLVM doesn't distinguish u/i in its type names). I was trying to help the confusion with this change, but we're not there yet... 🙃

Suggested change

/// That function must take no arguments and return an LLVM `i64`.

/// That function must take no arguments and return a Hugr `u64` (compiled to an LLVM `i64`).

No, hang on - Hugr doesn't distinguish these either, does it? So just exec_hugr_i64?

In fact this works fine for a Hugr returning, say, an int<0> (i.e. 1-bit int) - presumably anything assignment-convertible to an LLVM i64 - so it should probably be called exec_hugr_int...

src/custom/int.rs

acl-cqc · 2024-09-11T10:41:14Z

src/custom/conversions.rs

+    #[rstest]
+    #[case("convert_u", 4)]
+    #[case("convert_s", 5)]
+    fn test_convert(mut llvm_ctx: TestContext, #[case] op_name: &str, #[case] width: u8) -> () {


nit: width -> log_width

mark-koch

Looks good to me! I'm fine with doing the conversion unconditionally and leaving out branching for now. But maybe create an issue so we can discuss and not forget about it.

The only thing I'm still confused by is the weird behaviour of the float_roundtrip test. No idea why you're getting 2^32. I'm happy to merge as is, but we should also create an issue to further investigate.

Also, it might be nice to have an execution test that covers the failure case. But only bother with this if it's not a big hassle since the IR looks ok

mark-koch · 2024-09-11T09:10:39Z

src/custom/conversions.rs

+        hugr_type: &CustomType,
+    ) -> Result<BasicTypeEnum<'c>> {
+        Err(anyhow!(
+            "IntOpsCodegenExtension: unsupported type: {}",


Suggested change

"IntOpsCodegenExtension: unsupported type: {}",

"ConversionsCodegenExtension: unsupported type: {}",

mark-koch · 2024-09-12T12:08:23Z

src/custom/conversions.rs

+            6 => (64, (i64::MIN, i64::MAX), u64::MAX),
+            m => {
+                return Err(anyhow!(
+                    "IntTypesCodegenExtension: unsupported log_width: {}",


Suggested change

"IntTypesCodegenExtension: unsupported log_width: {}",

"ConversionEmitter: unsupported log_width: {}",

## 🤖 New release * `hugr-llvm`: 0.4.0 -> 0.5.0 <details><summary>Changelog</summary> <blockquote> ## [0.5.0](v0.4.0...v0.5.0) - 2024-09-16 ### New Features - Add emitters for int <-> float/usize conversions ([#94](#94)) - [**breaking**] array ops ([#96](#96)) - Add conversions itobool, ifrombool ([#101](#101)) - Add `tket2` feature and lowerings for `tket2.rotation` extension ([#100](#100)) ### Testing - Add execution test framework ([#97](#97)) </blockquote> </details> --- This PR was generated with [release-plz](https://github.com/MarcoIeni/release-plz/).

feat: Add emitters for int <-> float conversions

c9a2ea6

croyzor requested a review from a team as a code owner September 3, 2024 13:58

croyzor requested a review from mark-koch September 3, 2024 13:58

croyzor commented Sep 3, 2024

View reviewed changes

src/emit/ops.rs Show resolved Hide resolved

mark-koch reviewed Sep 3, 2024

View reviewed changes

croyzor and others added 21 commits September 3, 2024 16:31

Update src/custom/conversions.rs

f0b52fc

Co-authored-by: Mark Koch <[email protected]>

Remove redundant import

21227e3

Add method to get current module

9df42ce

Redo the logic

f135ba0

Clippy

2c1ac0d

cargo fmt

429410e

Fix a bug

d2ea3b1

Get log width from type args

9864b5e

Refactor into a method; add sign extension; build int_max from rust

fdacf48

clippy

128b909

Add missing int arg to iabs

3fbe422

fix: pack truncation result in a struct

b06b62b

Update snapshots

272cb7a

fmt

c3150bb

Update binary builder to be consistent with the unary one

ff914b2

Merge remote-tracking branch 'origin/main' into feat/conversions

33d6dbf

Fix a bug

79d458c

zext does what i thought sext did!

Replace poison with undef

cfd8ef0

Update snapshots

fe9e0fe

Add implementation of u64 <-> usize

e15a363

Add execution tests

d517c08

croyzor force-pushed the feat/conversions branch from 045257b to d517c08 Compare September 9, 2024 14:28

croyzor changed the title ~~feat: Add emitters for int <-> float conversions~~ feat: Add emitters for int <-> float/usize conversions Sep 9, 2024

croyzor requested a review from mark-koch September 9, 2024 14:30

acl-cqc reviewed Sep 10, 2024

View reviewed changes

src/custom/int.rs Show resolved Hide resolved

croyzor added 6 commits September 10, 2024 16:56

Wee refactor

98195d8

Statically compute the f64 upper bound

a1ee70a

Don't rely on our current sum type representation

7742e1d

Update snapshots

5504734

Check lower bounds for truncating floats; get rid of fabs

327fa72

Review comments

541277b

croyzor requested a review from mark-koch September 11, 2024 08:59

acl-cqc reviewed Sep 11, 2024

View reviewed changes

Rename width -> log_width

3ed1c8b

acl-cqc mentioned this pull request Sep 11, 2024

feat: Add conversions itobool, ifrombool #101

Merged

mark-koch approved these changes Sep 12, 2024

View reviewed changes

croyzor mentioned this pull request Sep 12, 2024

Consider doing bounds checks and branching on the result CQCL/hugr#1705

Closed

croyzor added 2 commits September 12, 2024 14:15

Review comments

fede030

Add approximate roundtrip tests

900b3a5

croyzor mentioned this pull request Sep 12, 2024

Investigate weird int -> float conversions #103

Closed

Cargo fmt

85be9c1

croyzor enabled auto-merge September 12, 2024 13:27

croyzor disabled auto-merge September 12, 2024 13:28

croyzor enabled auto-merge September 12, 2024 13:28

croyzor added this pull request to the merge queue Sep 12, 2024

Merged via the queue into main with commit 1286f60 Sep 12, 2024
10 checks passed

croyzor deleted the feat/conversions branch September 12, 2024 13:30

hugrbot mentioned this pull request Sep 12, 2024

chore: release v0.5.0 #98

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add emitters for int <-> float/usize conversions #94

feat: Add emitters for int <-> float/usize conversions #94

croyzor commented Sep 3, 2024 •

edited

Loading

codecov-commenter commented Sep 3, 2024 •

edited

Loading

acl-cqc Sep 10, 2024

acl-cqc Sep 10, 2024

croyzor Sep 11, 2024

acl-cqc Sep 10, 2024

acl-cqc Sep 10, 2024

croyzor Sep 10, 2024

acl-cqc Sep 11, 2024

acl-cqc Sep 11, 2024

acl-cqc Sep 11, 2024

acl-cqc Sep 11, 2024

mark-koch left a comment

mark-koch Sep 11, 2024

mark-koch Sep 12, 2024

	/// `entry_point` in the inner module.
	/// by `entry_point` in the inner module.

	/// That function must take no arguments and return an LLVM `i64`.
	/// That function must take no arguments and return a Hugr `u64` (compiled to an LLVM `i64`).

	"IntOpsCodegenExtension: unsupported type: {}",
	"ConversionsCodegenExtension: unsupported type: {}",

	"IntTypesCodegenExtension: unsupported log_width: {}",
	"ConversionEmitter: unsupported log_width: {}",

feat: Add emitters for int <-> float/usize conversions #94

feat: Add emitters for int <-> float/usize conversions #94

Conversation

croyzor commented Sep 3, 2024 • edited Loading

codecov-commenter commented Sep 3, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mark-koch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

croyzor commented Sep 3, 2024 •

edited

Loading

codecov-commenter commented Sep 3, 2024 •

edited

Loading