dialects: (builtin) normalize signless values on init #3554

superlopuh · 2024-12-02T12:19:30Z

Follows MLIR behaviour.

jorendumoulin

Nice!

xdsl/dialects/builtin.py

jorendumoulin · 2024-12-02T14:04:20Z

xdsl/dialects/builtin.py

+            if any(signed_ub <= attr.data for attr in attr_list):
+                attr_list = tuple(
+                    IntAttr(data_type.normalize_value(attr.data)) for attr in attr_list
+                )


Is there any harm in just doing this every time? Otherwise I would think an additional check for too negative numbers is also required.

an extra filecheck for this case is maybe also needed

What do you mean by doing it every time?

omit this check: if any(signed_ub <= attr.data for attr in attr_list):

I agree, isn't normalize_value doing the same checks anyway?

I generally dislike the idea of constructors being written in a way that doesn't allow the user to pass in correct data that is not copied/transformed in some way. I'll rewrite this part to conditionally normalize but return the original IntAttr if there's nothing to change.

superlopuh · 2024-12-02T14:32:02Z

tests/filecheck/backend/csl/print_csl.mlir

@@ -649,7 +649,7 @@ csl.func @builtins() {
 // CHECK-NEXT:   return;
 // CHECK-NEXT: }
 // CHECK-NEXT: comptime {
-// CHECK-NEXT:   @bind_control_task(control_task, @get_control_task_id(42));
+// CHECK-NEXT:   @bind_control_task(control_task, @get_control_task_id(-22));


@n-io, will this still work? Should the values here be always printed as positive integers by the csl printer?

Not quite sure about this change? It should prob not print negative values.

OK I'll attempt to fix the CSL printer then, as the signless before this change may already be in the negative range.

tests/filecheck/parser-printer/builtin_attrs.mlir

tests/irdl/test_declarative_assembly_format.py

codecov · 2024-12-02T14:37:02Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.40%. Comparing base (dd58ef0) to head (c75d736).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3554   +/-   ##
=======================================
  Coverage   90.40%   90.40%           
=======================================
  Files         469      469           
  Lines       58907    58941   +34     
  Branches     5605     5611    +6     
=======================================
+ Hits        53252    53287   +35     
  Misses       4213     4213           
+ Partials     1442     1441    -1

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

alexarice

I think this likely subtly changes a number of existing passes, which compare integers by grabbing the data out of an IntegerAttr. I've caught one in the cf canonicalization patterns but I'm not what can be done to catch this in general.

alexarice · 2024-12-02T14:38:25Z

tests/dialects/test_builtin.py

@@ -58,6 +60,51 @@ def test_IntegerType_size():
    assert IntegerType(64).size == 8


+def test_IntegerType_normalize():
+    s8 = IntegerType(8, Signedness.SIGNED)


Extreme nit but I believe si is usually used in mlir as a prefix for signed integers

oh this is great, I couldn't remember it, and it's not in the IntegerAttr doc, of course

alexarice · 2024-12-02T14:41:48Z

tests/dialects/test_builtin.py

+
+    assert s8.normalize_value(-1) == -1
+    assert s8.normalize_value(1) == 1
+    assert s8.normalize_value(255) == 255


I'm not sure I understand the semantics here, do we not change values that fall outside the range?

Yep, and your comment means that I should have documented this better: the idea is that we want to first preserve the behaviour for invalid integers, to raise a VerifyException with the invalid value. If the valid value is ambiguous due to signlessness, only then normalize to the signed range. I'll add this somewhere.

add more explanation to doc comment

tests/dialects/test_builtin.py

xdsl/dialects/builtin.py

alexarice · 2024-12-02T14:49:25Z

xdsl/dialects/builtin.py

+            return value
+
+        signed_ub = signed_upper_bound(self.bitwidth)
+        unsigned_ub = signed_ub << 1


Suggested change

unsigned_ub = signed_ub << 1

unsigned_ub = unsigned_upper_bound(self.bitwidth)

Why not use this? There will be complications with i0 right?

not really, this was a micro-optimisation, but I guess not really very important

alexarice · 2024-12-02T14:54:13Z

xdsl/transforms/canonicalization_patterns/cf.py

            rewriter.replace_matched_op(cf.BranchOp(op.then_block, *op.then_arguments))
-        elif cond == 0:
+        else:
            rewriter.replace_matched_op(cf.BranchOp(op.else_block, *op.else_arguments))


I can't work out how to attach the comment in the correct place but line 374 of this file will now be incorrect

alexarice · 2024-12-02T14:55:08Z

xdsl/dialects/builtin.py

+            if any(signed_ub <= attr.data for attr in attr_list):
+                attr_list = tuple(
+                    IntAttr(data_type.normalize_value(attr.data)) for attr in attr_list
+                )


I agree, isn't normalize_value doing the same checks anyway?

superlopuh · 2024-12-02T16:03:42Z

I'm converting to draft as this seems like quite large a change, I'll split this up further by the seemingly unrelated things that are failing.

Follows MLIR behaviour.

superlopuh added the dialects Changes on the dialects label Dec 2, 2024

superlopuh requested review from zero9178, alexarice, math-fehr, Moxinilian and jorendumoulin December 2, 2024 12:19

superlopuh self-assigned this Dec 2, 2024

jorendumoulin approved these changes Dec 2, 2024

View reviewed changes

superlopuh commented Dec 2, 2024

View reviewed changes

tests/filecheck/parser-printer/builtin_attrs.mlir Outdated Show resolved Hide resolved

superlopuh commented Dec 2, 2024

View reviewed changes

tests/irdl/test_declarative_assembly_format.py Outdated Show resolved Hide resolved

alexarice reviewed Dec 2, 2024

View reviewed changes

superlopuh force-pushed the sasha/builtin/normalise-value branch from 9261fe6 to 6609edb Compare December 2, 2024 15:20

superlopuh changed the base branch from sasha/builtin/0-bitwidth-ranges to sasha/builtin/print-i1 December 2, 2024 15:20

superlopuh marked this pull request as draft December 2, 2024 16:03

superlopuh force-pushed the sasha/builtin/print-i1 branch from 035fc8a to 061e87c Compare December 2, 2024 16:14

superlopuh force-pushed the sasha/builtin/normalise-value branch from 9de5b32 to f2d97a7 Compare December 2, 2024 16:14

Base automatically changed from sasha/builtin/print-i1 to main December 2, 2024 17:04

superlopuh added 6 commits December 2, 2024 17:08

dialects: (builtin) add get_value helper to IntegerAttr

9e091f4

dialects: (builtin) normalize signless values on init

48664b2

fix bugs

661fc0b

si8

9d7b19a

update doc comment

6cac767

empty line

bbda01f

superlopuh force-pushed the sasha/builtin/normalise-value branch from f2d97a7 to bbda01f Compare December 2, 2024 17:13

superlopuh changed the base branch from main to sasha/builtin/py-value December 2, 2024 17:13

Base automatically changed from sasha/builtin/py-value to main December 5, 2024 08:29

Merge branch 'main' into sasha/builtin/normalise-value

18eb875

superlopuh added 3 commits December 5, 2024 08:31

revert csl changes

d49f613

remove py_value

fd34f12

normalize int attributes instead

0413f71

superlopuh marked this pull request as ready for review December 5, 2024 16:08

use unsigned_upper_bound

3fcd495

math-fehr approved these changes Dec 5, 2024

View reviewed changes

Merge branch 'main' into sasha/builtin/normalise-value

c75d736

superlopuh merged commit d4f5795 into main Dec 6, 2024
15 checks passed

superlopuh deleted the sasha/builtin/normalise-value branch December 6, 2024 00:47

EdmundGoodman pushed a commit to EdmundGoodman/xdsl that referenced this pull request Dec 6, 2024

dialects: (builtin) normalize signless values on init (xdslproject#3554)

833e710

Follows MLIR behaviour.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dialects: (builtin) normalize signless values on init #3554

dialects: (builtin) normalize signless values on init #3554

superlopuh commented Dec 2, 2024 •

edited

Loading

jorendumoulin left a comment

jorendumoulin Dec 2, 2024

jorendumoulin Dec 2, 2024

superlopuh Dec 2, 2024

jorendumoulin Dec 2, 2024

alexarice Dec 2, 2024

superlopuh Dec 4, 2024

superlopuh Dec 2, 2024

n-io Dec 2, 2024

superlopuh Dec 2, 2024 •

edited

Loading

codecov bot commented Dec 2, 2024 •

edited

Loading

alexarice left a comment

alexarice Dec 2, 2024

superlopuh Dec 2, 2024

alexarice Dec 2, 2024

superlopuh Dec 2, 2024 •

edited

Loading

alexarice Dec 2, 2024

superlopuh Dec 2, 2024

alexarice Dec 2, 2024

alexarice Dec 2, 2024

superlopuh commented Dec 2, 2024

	unsigned_ub = signed_ub << 1
	unsigned_ub = unsigned_upper_bound(self.bitwidth)

dialects: (builtin) normalize signless values on init #3554

dialects: (builtin) normalize signless values on init #3554

Conversation

superlopuh commented Dec 2, 2024 • edited Loading

jorendumoulin 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

superlopuh Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Dec 2, 2024 • edited Loading

Codecov Report

alexarice 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

superlopuh Dec 2, 2024 • edited Loading

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

superlopuh commented Dec 2, 2024

superlopuh commented Dec 2, 2024 •

edited

Loading

superlopuh Dec 2, 2024 •

edited

Loading

codecov bot commented Dec 2, 2024 •

edited

Loading

superlopuh Dec 2, 2024 •

edited

Loading