feat: Constant-folding of integer and logic operations

[cqc-alec]

Add constant folders for all integer and logical operations, and add tests. Includes a few small refactors and fixes.

One test (which I'd intended to expand into a larger one involving many operations) is marked "should panic" because of #996 .

Closes #773 .

[codecov]

Codecov Report

Attention: Patch coverage is 88.44971% with 263 lines in your changes are missing coverage. Please review.

Project coverage is 86.25%. Comparing base (3370d88) to head (0efe5a4).

Files	Patch %	Lines
...rc/std_extensions/arithmetic/int_ops/const_fold.rs	74.92%	131 Missing and 120 partials ⚠️
...td_extensions/arithmetic/conversions/const_fold.rs	0.00%	2 Missing and 1 partial ⚠️
hugr/src/ops/constant/custom.rs	83.33%	1 Missing and 1 partial ⚠️
hugr/src/std_extensions/arithmetic/conversions.rs	71.42%	2 Missing ⚠️
hugr/src/std_extensions/arithmetic/int_ops.rs	94.28%	0 Missing and 2 partials ⚠️
hugr/src/std_extensions/logic.rs	75.00%	1 Missing and 1 partial ⚠️
hugr/src/utils.rs	90.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1009      +/-   ##
==========================================
+ Coverage   85.71%   86.25%   +0.53%     
==========================================
  Files          79       81       +2     
  Lines       14639    16848    +2209     
  Branches    14639    16848    +2209     
==========================================
+ Hits        12548    14532    +1984     
- Misses       1434     1536     +102     
- Partials      657      780     +123     

Flag	Coverage Δ
rust	86.25% <88.44%> (+0.53%)	⬆️

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.

[doug-q]

Wow huge job!

As I mentioned in many inline comments #1002 will make a few things a little bit nicer.

I think we should pay attention to the coverage report here, which seems to show ~15% of the constant folding missing coverage. I can't seem to view it properly though, so I may be mistaken. In any case, let's make sure we cover all these foldings.

I feel like you could shorten your test cases a lot by writing something like

fn const_fold_test(expected_values: &[Value], reg: &ExtensionRegistry, f: impl FnOnce(&mut DFGBuilder) -> Vec<Wire>)  {
   let mut builder = todo!();
   let out_wires = f(builder);
   let mut h = builder.finish_hugr_with_outputs(out_wires, reg).unwrap();
   constant_fold_pass(&mut h, reg);
   assert_fully_folded(&h, &expected_values);
}

I have not tried this, maybe you would need several? In any case it might reduce the noise a bit. You could also create some(or maybe one is enough?) static ExtensionRegistrys and reuse them

[doug-q]

assuming #1002 lands as-is, these can change to int_type(0)

[doug-q]

again assuming #1004 lands, (n as u64).into()

[doug-q]

Suggested change

	let log_widths: Vec<u8> = args.iter().map(|a| get_log_width(a).unwrap()).collect();
	let log_widths: Vec<u8> = args.iter().map(|a| get_log_width(a)).collect::<Result<_>>()?;

I think that's right... it's certainly possible to collect into a Result like this. It's pretty bad to unwrap here.

[cqc-alec]

error[E0107]: enum takes 2 generic arguments but 1 generic argument was supplied
   --> hugr/src/std_extensions/arithmetic/int_ops.rs:306:83
    |
306 | ...t_log_width(a)).collect::<Result<_>>()?;
    |                              ^^^^^^ - supplied 1 generic argument
    |                              |
    |                              expected 2 generic arguments
    |

If I change to Result<_, _>:

error[E0277]: `?` couldn't convert the error to `OpLoadError`
   --> hugr/src/std_extensions/arithmetic/int_ops.rs:306:98
    |
306 | ...t<_, _>>()?;
    |              ^ the trait `From<TypeArgError>` is not implemented for `OpLoadError`
    |

[doug-q]

See my PR.

[doug-q]

Suggested change

	pub fn with_log_width(self, width: u8) -> IntOpType {
	pub fn with_log_width(self, log_width: u8) -> IntOpType {

[cqc-alec]

Done,

[doug-q]

Suggested change

	log_widths: vec![width],
	log_widths: vec![log_width],

[cqc-alec]

Done.

[doug-q]

Suggested change

	log_widths: vec![first_width, second_width],
	log_widths: vec![first_log_width, second_log_width],

[cqc-alec]

Done.

[doug-q]

fn ta can be removed, pass the u64 straight to int_type

[cqc-alec]

Apparently not:

   --> hugr/src/std_extensions/arithmetic/int_ops.rs:382:45
    |
382 |             FunctionType::new(vec![int_type(3)], vec![int_type(4)],)
    |                                    -------- ^ expected `TypeArg`, found integer
    |                                    |
    |                                    arguments to this function are incorrect
    |

[doug-q]

See my PR.

[doug-q]

This needs some tests

[cqc-alec]

Agreed, added.

[doug-q]

recommend make this pub(crate)

[cqc-alec]

I've made it public in the test moduile in utils.

[doug-q]

I think all these tests should be moved to a new file src/std_extensions/arithmetic/int_ops/const_fold/test.rs`

[cqc-alec]

Done.

[doug-q]

You have a redundant mod test inside the new file. See my PR

[doug-q]

I think we should pay attention to the coverage report here, which seems to show ~15% of the constant folding missing coverage. I can't seem to view it properly though, so I may be mistaken. In any case, let's make sure we cover all these foldings.

On further inspection, the missing coverage is all in error-handling, which is fine by me. Nice!

[doug-q]

@cqc-alec see #1014 for fixes to the problems you reported. Note it says it's not merging cleanly. I expect this is because of the file-wide indentation change in test.rs. I recommend you merge main here, then cherry-pick the non-merge commits from that PR here.

[cqc-alec]

@cqc-alec see #1014 for fixes to the problems you reported. Note it says it's not merging cleanly. I expect this is because of the file-wide indentation change in test.rs. I recommend you merge main here, then cherry-pick the non-merge commits from that PR here.

Thanks @doug-q ! I've adopted your suggestions except for the test code refactoring. While it might reduce the code size, I don't think repetition in test code is actually a bad thing: it makes it easier to understand what a test is doing by looking at a single chunk of code; and it also makes it easier to modify and expand individual tests. (For example, the fact that so many of the tests worked with 32-bit integers is more laziness on my part than something I'd want to bake in.)

[doug-q]

@cqc-alec see #1014 for fixes to the problems you reported. Note it says it's not merging cleanly. I expect this is because of the file-wide indentation change in test.rs. I recommend you merge main here, then cherry-pick the non-merge commits from that PR here.

Thanks @doug-q ! I've adopted your suggestions except for the test code refactoring. While it might reduce the code size, I don't think repetition in test code is actually a bad thing: it makes it easier to understand what a test is doing by looking at a single chunk of code; and it also makes it easier to modify and expand individual tests. (For example, the fact that so many of the tests worked with 32-bit integers is more laziness on my part than something I'd want to bake in.)

Great, I've no problem with that

[doug-q]

Awesome