dialects: (builtin) add IntegerAttr truncation and use in individual rewrite #3585

alexarice · 2024-12-06T15:19:14Z

AdditionOfSameVariablesToMultiplyByTwo has a corner case where the bitwidth is 1. This small change fixes this issue.

alexarice · 2024-12-06T15:21:29Z

@superlopuh should this be dealt with by normalization?

codecov · 2024-12-06T15:25:19Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.43%. Comparing base (b5acf31) to head (123638a).
Report is 27 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3585      +/-   ##
==========================================
+ Coverage   90.40%   90.43%   +0.02%     
==========================================
  Files         469      472       +3     
  Lines       58950    59301     +351     
  Branches     5611     5639      +28     
==========================================
+ Hits        53296    53629     +333     
- Misses       4212     4228      +16     
- Partials     1442     1444       +2

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

superlopuh · 2024-12-06T16:07:56Z

This is a classic, I believe CC @math-fehr . I think this particular case will be dealt with with normalization

superlopuh · 2024-12-06T16:08:14Z

It would be good to have a filecheck for this

math-fehr

Question, why was the previous implementation wrong?
With an i1, the constant 2 should be folded to 0 right?
So x + x gets folded to x * 0, which is correct AFAIK?

alexarice · 2024-12-07T16:46:23Z

Question, why was the previous implementation wrong? With an i1, the constant 2 should be folded to 0 right? So x + x gets folded to x * 0, which is correct AFAIK?

It doesn't get converted to 0, it just adds an i1 with 2 inside which then fails a verify check

alexarice · 2024-12-09T10:12:34Z

@math-fehr @superlopuh any idea how to proceed here? Would it be better to always wrap integers to their range, or at least have an IntegerAttr initialiser that does this?

xdsl/transforms/individual_rewrite.py

superlopuh · 2024-12-09T10:46:07Z

Another possible solution is to add a truncating constructor to accept any size of input

alexarice · 2024-12-09T10:47:27Z

This is what I meant by having a wrapping IntegerAttr initialiser

math-fehr · 2024-12-09T14:54:12Z

I would agree with the wrapping IntegerAttr init

alexarice · 2024-12-09T17:09:29Z

I've pushed a version which adds an option to truncate an IntegerAttr. If this seems reasonable I can potentially split that into a separate PR or at least add some unit tests.

superlopuh · 2024-12-09T18:09:58Z

xdsl/dialects/builtin.py


-        if self.signedness.data == Signedness.SIGNLESS:
+        if self.signedness.data != Signedness.UNSIGNED:


why this change?

Doing the bitwise and seems to wrap everything into the range [0,2**bitwidth), so for the signed case we need to normalise it back to the correct range. This seemed like the easiest way to do this but it could be refactored

superlopuh

I'm not sure that these are entirely composable things, and signedness complicates things. I'm not sure how to best do this in the general case, handling all possible signednesses and scenarios. Would it make sense to change this PR to only handle the i1 case in the pattern, and then start a separate discussion on how to truncate integers?

alexarice · 2024-12-09T18:55:23Z

I don't really see how this isn't composable into

Add optional truncation for IntegerAttr (with added tests)
Enable the flag on this test.

I am also not sure I understand the concerns around signedness

superlopuh

I played around with the code locally, and I get how it works a bit better now, happy to go with this direction. Could you please add unit tests for the helper, and a filechec test for the canonicalization pattern?

alexarice · 2024-12-09T19:40:42Z

I've realised I can probably replace the & (2**8 - 1) with % 2**8, which acts exactly the same as far as I can tell and is way more readable

superlopuh · 2024-12-10T10:15:37Z

tests/filecheck/transforms/individual_rewrite/add-same.mlir

+// CHECK:      %[[#var:]] = "test.op"() : () -> i32
+// CHECK-NEXT: %[[#two:]] = arith.constant 2 : i32
+// CHECK-NEXT: %{{.*}} = arith.muli %[[#var]], %[[#two]] : i32
+
+%0 = "test.op"() : () -> (i32)
+%1 = arith.addi %0, %0 : i32
+
+// -----
+
+// CHECK:      %[[#var:]] = "test.op"() : () -> i1
+// CHECK-NEXT: %[[#zero:]] = arith.constant false
+// CHECK-NEXT: %{{.*}} = arith.muli %[[#var]], %[[#zero]] : i1
+
+%0 = "test.op"() : () -> (i1)
+%1 = arith.addi %0, %0 : i1


nit: are the regexes here really more readable than giving names to ssa values and checking for those directly?

I've removed 1, but can't/haven't named the constants

If you name the inputs, the constants they are replaced with inherit their hints, but this is more of a nit at this point

I don't want the test to be checking that the constant has the same name as the original operation, and so would prefer to use the regex

math-fehr

Nice! Looks good to me!

math-fehr · 2024-12-10T14:47:48Z

xdsl/dialects/builtin.py

@@ -397,21 +397,26 @@ def verify_value(self, value: int):
                f"values in the range [{min_value}, {max_value})"
            )

-    def normalized_value(self, value: IntAttr) -> IntAttr | None:
+    def normalized_value(
+        self, value: IntAttr, *, truncate_bits: bool = False


I think a name like wrapping would be clearer than truncate_bits.

Hmm, what's it wrapping?

Wrapping the int into the range. For what it's worth the overflow flags use the word wrap

Yes, and truncating has already a meaning in LLVM/arith, which is to go from let's say i32 to i16, which is not really what we are doing here

But wrapping is a feature of the operation, not of a value normalization step. I'm happy for an addition to be wrapping, but it seems a bit out of context here.

Here's the relevant API that I have most experience with https://developer.apple.com/documentation/swift/int/init(truncating:)

Did we come to a conclusion?

We can hash it out on the zulip if you want, I'm happy to be outvoted if you both think wrapping is more intuitive here

transforms: (individual-rewrite) fix i1 case

1f3bd5c

alexarice added bug Something isn't working minor For minor PRs, easy and quick to review, quickly mergeable transformations Changes or adds a transformatio labels Dec 6, 2024

alexarice requested review from superlopuh and dshaaban01 December 6, 2024 15:19

alexarice self-assigned this Dec 6, 2024

math-fehr reviewed Dec 7, 2024

View reviewed changes

superlopuh reviewed Dec 9, 2024

View reviewed changes

xdsl/transforms/individual_rewrite.py Outdated Show resolved Hide resolved

use truncate_bits

e08a833

alexarice requested review from math-fehr and superlopuh December 9, 2024 17:09

superlopuh reviewed Dec 9, 2024

View reviewed changes

alexarice added 3 commits December 10, 2024 09:03

Add filecheck test

0996e35

Switch truncate implementation

cae1259

Add tests for truncation

2a8f874

alexarice changed the title ~~transforms: (individual-rewrite) fix i1 case~~ dialects: (builtin) add IntegerAttr truncation and use in individual rewrite Dec 10, 2024

alexarice requested a review from superlopuh December 10, 2024 09:13

alexarice mentioned this pull request Dec 10, 2024

dialects: (arith) add SignlessIntegerBinaryOperation canonicalization #3583

Draft

superlopuh reviewed Dec 10, 2024

View reviewed changes

superlopuh approved these changes Dec 10, 2024

View reviewed changes

Remove 1 regex

123638a

math-fehr approved these changes Dec 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dialects: (builtin) add IntegerAttr truncation and use in individual rewrite #3585

dialects: (builtin) add IntegerAttr truncation and use in individual rewrite #3585

alexarice commented Dec 6, 2024

alexarice commented Dec 6, 2024

codecov bot commented Dec 6, 2024 •

edited

Loading

superlopuh commented Dec 6, 2024

superlopuh commented Dec 6, 2024

math-fehr left a comment

alexarice commented Dec 7, 2024

alexarice commented Dec 9, 2024

superlopuh commented Dec 9, 2024

alexarice commented Dec 9, 2024

math-fehr commented Dec 9, 2024

alexarice commented Dec 9, 2024

superlopuh Dec 9, 2024

alexarice Dec 9, 2024

superlopuh left a comment

alexarice commented Dec 9, 2024

superlopuh left a comment

alexarice commented Dec 9, 2024

superlopuh Dec 10, 2024

alexarice Dec 10, 2024

superlopuh Dec 10, 2024

alexarice Dec 12, 2024

math-fehr left a comment

math-fehr Dec 10, 2024

superlopuh Dec 10, 2024

alexarice Dec 10, 2024

math-fehr Dec 10, 2024

superlopuh Dec 10, 2024

superlopuh Dec 10, 2024

alexarice Dec 12, 2024

superlopuh Dec 12, 2024


		if self.signedness.data == Signedness.SIGNLESS:
		if self.signedness.data != Signedness.UNSIGNED:

dialects: (builtin) add IntegerAttr truncation and use in individual rewrite #3585

Are you sure you want to change the base?

dialects: (builtin) add IntegerAttr truncation and use in individual rewrite #3585

Conversation

alexarice commented Dec 6, 2024

alexarice commented Dec 6, 2024

codecov bot commented Dec 6, 2024 • edited Loading

Codecov Report

superlopuh commented Dec 6, 2024

superlopuh commented Dec 6, 2024

math-fehr left a comment

Choose a reason for hiding this comment

alexarice commented Dec 7, 2024

alexarice commented Dec 9, 2024

superlopuh commented Dec 9, 2024

alexarice commented Dec 9, 2024

math-fehr commented Dec 9, 2024

alexarice commented Dec 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

superlopuh left a comment

Choose a reason for hiding this comment

alexarice commented Dec 9, 2024

superlopuh left a comment

Choose a reason for hiding this comment

alexarice commented Dec 9, 2024

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

math-fehr left a 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

codecov bot commented Dec 6, 2024 •

edited

Loading