dialects: (llvm) Registering llvm.icmp #3356

lfrenot · 2024-10-30T11:37:39Z

No description provided.

tobiasgrosser

Otherwise, this looks great to me. Maybe @superlopuh and @luisacicolini have comments?

tests/filecheck/dialects/llvm/arithmetic.mlir

lfrenot · 2024-10-30T11:42:21Z

I'm getting this error

# |     raise ParseError(at_position, msg)
# | xdsl.utils.exceptions.ParseError: stdin:26:72
# |   %icmp_eq = "llvm.icmp"(%arg0, %arg1) <{"predicate" = #llvm.predicateeq}> : (i32, i32) -> i32
# |                                                                         ^
# |                                                                         'llvm.predicateeq' is not registered

Since the predicate is supposed to be an i64 (according to mlir-opt --mlir-print-op-generic), I guess I have to change the way I'm representing it right now

superlopuh · 2024-10-30T11:43:59Z

xdsl/dialects/llvm.py

+        if i == 0:
+            return ICmpPredicateFlag.EQ
+        elif i == 1:
+            return ICmpPredicateFlag.NE
+        elif i == 2:
+            return ICmpPredicateFlag.SLT
+        elif i == 3:
+            return ICmpPredicateFlag.SLE
+        elif i == 4:
+            return ICmpPredicateFlag.SGE
+        elif i == 5:
+            return ICmpPredicateFlag.SGT
+        elif i == 6:
+            return ICmpPredicateFlag.ULT
+        elif i == 7:
+            return ICmpPredicateFlag.ULE
+        elif i == 8:
+            return ICmpPredicateFlag.UGE
+        elif i == 9:
+            return ICmpPredicateFlag.UGT


There has to be a better way of doing this, I'd expect some helper to exist in the Python enum implementation, otherwise we should cache this mapping.

superlopuh · 2024-10-30T12:19:06Z

This PR adds an operation and an attribute, and only registers the new op in the dialect.

lfrenot · 2024-10-30T12:43:24Z

# | ValueError: 0 is not a valid ICmpPredicateFlag

I don't get why I'm getting this error with the current def of ICmpPredicateFlag

codecov · 2024-10-30T14:31:26Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.12%. Comparing base (e6c7282) to head (ee7b874).
Report is 6 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3356      +/-   ##
==========================================
+ Coverage   90.10%   90.12%   +0.01%     
==========================================
  Files         449      449              
  Lines       56599    56693      +94     
  Branches     5429     5434       +5     
==========================================
+ Hits        51001    51092      +91     
- Misses       4164     4166       +2     
- Partials     1434     1435       +1

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

lfrenot · 2024-10-30T14:54:41Z

@superlopuh
Does it look good for you?
Do you know how to properly do the enum thing?

superlopuh · 2024-10-30T15:57:58Z

xdsl/dialects/llvm.py

+    lhs = operand_def(T)
+    rhs = operand_def(T)
+    res = result_def(T)
+    predicate = prop_def(Attribute)


why this change? it was better before as a strenum, no?

superlopuh · 2024-10-30T15:58:32Z

I would recommend reverting your last change, and looking into how to index the enum cases

superlopuh · 2024-10-30T16:00:04Z

from enum import StrEnum


class MyEnum(StrEnum):
    a = "a"
    b = "b"


MY_ENUM_CASES = tuple(MyEnum)

print(MY_ENUM_CASES[1])

This seems to work

superlopuh · 2024-10-30T16:00:16Z

and is not O(number of cases)

xdsl/dialects/llvm.py

lfrenot · 2024-10-31T09:26:20Z

@superlopuh do you have any idea on how to fix the current error?

Co-authored-by: Sasha Lopoukhine <[email protected]>

superlopuh · 2024-10-31T09:26:31Z

Ah no, it looks like I was too quick to approve. I would recommend marking the PR as draft if the tests don't pass, and reaching out on Zulip to iterate

superlopuh · 2024-10-31T09:29:20Z

I've actually never used this part of the API before, I'm happy to take a look in a bit if you're stuck, but it would be worth messaging on Zulip in case someone with more experience with your issue knows the answer directly. In this case pinging @alexarice would probably be a good idea, I think he's dealt with these attributes somewhat recently.

alexarice · 2024-10-31T10:40:56Z

xdsl/dialects/llvm.py

+
+
+@irdl_attr_definition
+class ICmpPredicateAttr(EnumAttribute[ICmpPredicateFlag]):


I believe you want OpaqueSyntaxAttribute or SpacedOpaqueSyntaxAttribute here to get it to print properly

alexarice · 2024-10-31T10:42:50Z

xdsl/dialects/llvm.py

+
+
+@irdl_op_definition
+class ICmpOp(IRDLOperation, ABC):


Why is this marked as an abstract class?

alexarice · 2024-10-31T10:45:27Z

xdsl/dialects/llvm.py

+        self.predicate.print_parameter(printer)
+
+    def print(self, printer: Printer):
+        printer.print(' "')


I think in general, print_string is preferred to print but this is a super nit

lfrenot · 2024-10-31T11:02:12Z

@alexarice Thank you for the suggestions, that worked

@superlopuh are we good to merge then?

alexarice

I just tested the mlir version and it does this:

bin/mlir-opt ../../test.mlir --mlir-print-op-generic
"builtin.module"() ({
  %0:2 = "test.op"() : () -> (i32, i32)
  %1 = "llvm.icmp"(%0#0, %0#1) <{predicate = 0 : i64}> : (i32, i32) -> i1
}) : () -> ()

I feel the generic output of your function is not the same. Unsure if this is a blocker but I just wanted to point it out.

superlopuh

LGTM! The issue Alex pointed out is worth looking into, but I don't want to block on that necessarily. It would be great if you could take a quick look at the generic syntax before merging, and comment here.

tobiasgrosser · 2024-11-01T03:18:36Z

Let's merge this for now. @lfrenot, can you still check @alexarice's comment.

tobiasgrosser · 2024-11-01T03:32:31Z

I just tested the mlir version and it does this:
bin/mlir-opt ../../test.mlir --mlir-print-op-generic
"builtin.module"() ({
  %0:2 = "test.op"() : () -> (i32, i32)
  %1 = "llvm.icmp"(%0#0, %0#1) <{predicate = 0 : i64}> : (i32, i32) -> i1
}) : () -> ()
I feel the generic output of your function is not the same. Unsure if this is a blocker but I just wanted to point it out.

@lfrenot, we should indeed be consistent with MLIR here. I just tried to use your icmp implementation to parse our test cases, but it failed as MLIR emits an integer attribute and you added your own attribute type. It would be great if this could be fixed.

Co-authored-by: Sasha Lopoukhine <[email protected]>

lfrenot added 2 commits October 30, 2024 11:03

registered ICmpOp

c91c615

first test case

2a0439d

lfrenot added the dialects Changes on the dialects label Oct 30, 2024

lfrenot requested a review from tobiasgrosser October 30, 2024 11:37

lfrenot self-assigned this Oct 30, 2024

tobiasgrosser approved these changes Oct 30, 2024

View reviewed changes

tests/filecheck/dialects/llvm/arithmetic.mlir Show resolved Hide resolved

superlopuh reviewed Oct 30, 2024

View reviewed changes

rest of the test cases

5e31e71

changed predicate to Attribute

daa4345

lfrenot added 2 commits October 30, 2024 14:25

working version

8ddde3a

ruff fix

cd9d2da

Merge remote-tracking branch 'origin/main' into llvm-icmp

ec4a742

superlopuh reviewed Oct 30, 2024

View reviewed changes

lfrenot added 2 commits October 31, 2024 09:05

Merge remote-tracking branch 'origin/main' into llvm-icmp

e58888f

trying to use ICmpPredicateAttr

0363a46

superlopuh reviewed Oct 31, 2024

View reviewed changes

xdsl/dialects/llvm.py Outdated Show resolved Hide resolved

superlopuh approved these changes Oct 31, 2024

View reviewed changes

Update xdsl/dialects/llvm.py

e144646

Co-authored-by: Sasha Lopoukhine <[email protected]>

alexarice reviewed Oct 31, 2024

View reviewed changes

lfrenot added 2 commits October 31, 2024 10:53

Alex Rice suggestions

865c9ca

SpacedOpaqueSyntaxAttribute

ee7b874

alexarice reviewed Oct 31, 2024

View reviewed changes

superlopuh approved these changes Oct 31, 2024

View reviewed changes

tobiasgrosser merged commit 05506a1 into main Nov 1, 2024
14 checks passed

tobiasgrosser deleted the llvm-icmp branch November 1, 2024 03:18

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

dialects: (llvm) Registering llvm.icmp (xdslproject#3356)

d9445a0

Co-authored-by: Sasha Lopoukhine <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dialects: (llvm) Registering llvm.icmp #3356

dialects: (llvm) Registering llvm.icmp #3356

lfrenot commented Oct 30, 2024

tobiasgrosser left a comment

lfrenot commented Oct 30, 2024

superlopuh Oct 30, 2024

superlopuh commented Oct 30, 2024

lfrenot commented Oct 30, 2024

codecov bot commented Oct 30, 2024 •

edited

Loading

lfrenot commented Oct 30, 2024

superlopuh Oct 30, 2024

superlopuh commented Oct 30, 2024

superlopuh commented Oct 30, 2024 •

edited

Loading

superlopuh commented Oct 30, 2024

lfrenot commented Oct 31, 2024

superlopuh commented Oct 31, 2024

superlopuh commented Oct 31, 2024

alexarice Oct 31, 2024

alexarice Oct 31, 2024

alexarice Oct 31, 2024

lfrenot commented Oct 31, 2024

alexarice left a comment

superlopuh left a comment

tobiasgrosser commented Nov 1, 2024

tobiasgrosser commented Nov 1, 2024



		@irdl_attr_definition
		class ICmpPredicateAttr(EnumAttribute[ICmpPredicateFlag]):

dialects: (llvm) Registering llvm.icmp #3356

dialects: (llvm) Registering llvm.icmp #3356

Conversation

lfrenot commented Oct 30, 2024

tobiasgrosser left a comment

Choose a reason for hiding this comment

lfrenot commented Oct 30, 2024

superlopuh Oct 30, 2024

Choose a reason for hiding this comment

superlopuh commented Oct 30, 2024

lfrenot commented Oct 30, 2024

codecov bot commented Oct 30, 2024 • edited Loading

Codecov Report

lfrenot commented Oct 30, 2024

superlopuh Oct 30, 2024

Choose a reason for hiding this comment

superlopuh commented Oct 30, 2024

superlopuh commented Oct 30, 2024 • edited Loading

superlopuh commented Oct 30, 2024

lfrenot commented Oct 31, 2024

superlopuh commented Oct 31, 2024

superlopuh commented Oct 31, 2024

alexarice Oct 31, 2024

Choose a reason for hiding this comment

alexarice Oct 31, 2024

Choose a reason for hiding this comment

alexarice Oct 31, 2024

Choose a reason for hiding this comment

lfrenot commented Oct 31, 2024

alexarice left a comment

Choose a reason for hiding this comment

superlopuh left a comment

Choose a reason for hiding this comment

tobiasgrosser commented Nov 1, 2024

tobiasgrosser commented Nov 1, 2024

codecov bot commented Oct 30, 2024 •

edited

Loading

superlopuh commented Oct 30, 2024 •

edited

Loading