This is an archive of the discontinued LLVM Phabricator instance.

[mlir][arith] Change the syntax of `arith.cmpi/f`
AbandonedPublic

Authored by Mogball on Aug 31 2022, 5:49 PM.

Details

Summary

This patch changes the syntax of arith.cmpi and arith.cmpf to be

%0 = arith.cmpi uge(%a, %b) {} : i32
%1 = arith.cmpf olt(%c, %d) {} : f32

The previous syntax was awkward because it made the predicate look like
it was an operand.

These regexes were used to make the majority of the changes

cmp(i|f) *"?([a-z]+)"? *, *([a-z%#A-Z\[\]\{\}\.\*_:0-9\+]+) *, *([a-z%#A-Z\[\]\{\}\.\*_:0-9\+]+)
cmp$1 $2($3, $4)

Followed by

cmp(i|f) *"?([a-z]+)"? *, *([a-z%#A-Z\[\]\{\}\.\*_:\+0-9]+) *
cmp$1 $2($3

The rest were cleaned up by hand by searching for cmp(i|f) *[a-z]+ *,

Diff Detail

Event Timeline

Mogball created this revision.Aug 31 2022, 5:49 PM
Herald added a reviewer: aartbik. · View Herald Transcript
Herald added a reviewer: ftynse. · View Herald Transcript
Herald added a reviewer: aartbik. · View Herald Transcript
Herald added a reviewer: sjarus. · View Herald Transcript
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
Mogball requested review of this revision.Aug 31 2022, 5:49 PM
rriddle accepted this revision.Aug 31 2022, 5:53 PM

+1 from me, I think this looks much cleaner than before. Maybe wait for another +1 though.

If you made the changes with regex, could you provide what you used? It greatly simplifies work for downstream users.

This revision is now accepted and ready to land.Aug 31 2022, 5:53 PM
aartbik accepted this revision.Aug 31 2022, 6:15 PM

Good for sparse

Mogball edited the summary of this revision. (Show Details)Aug 31 2022, 6:29 PM

I put the regexes in the description

Mogball edited the summary of this revision. (Show Details)Aug 31 2022, 6:32 PM
jsetoain accepted this revision.Sep 1 2022, 12:48 AM

Nice! 🙂

ftynse added a comment.Sep 1 2022, 1:35 AM

I am going to go against the majority opinion here and say the syntax should just stay as it is. Most operations don't have their operands in parentheses and have their mandatory attributes as keywords. Introducing a less common syntax breaks the reading flow and unnecessarily anchors attention. The only reason why I think the current syntax may give the impression of the predicate being an operand is the comma between the predicate and the operand: arith.cmpi eq, %a, %b as opposed to arith.cmpi eq %a, %b. I also think that even rudimentary familiarity with the IR structure is sufficient to understand that the predicate is not in fact an operand: operands are values, and values are always prefixed with a % sign. On the other hand, the amount of churn required to essentially remove a comma is scary. For me, it's not worth it.

I am going to go against the majority opinion here and say the syntax should just stay as it is. Most operations don't have their operands in parentheses and have their mandatory attributes as keywords. Introducing a less common syntax breaks the reading flow and unnecessarily anchors attention. The only reason why I think the current syntax may give the impression of the predicate being an operand is the comma between the predicate and the operand: arith.cmpi eq, %a, %b as opposed to arith.cmpi eq %a, %b. I also think that even rudimentary familiarity with the IR structure is sufficient to understand that the predicate is not in fact an operand: operands are values, and values are always prefixed with a % sign.

Yeah, now that you mention it, it really is just the comma. I kind of like your suggestion better.

On the other hand, the amount of churn required to essentially remove a comma is scary. For me, it's not worth it.

I understand where you're coming from ( ;) ). I may be alone in this opinion, but I'm not comfortable with the idea that we shouldn't do things just because of churn. Over a comma (and a few braces), however, I'm willing to relent.

I am going to go against the majority opinion here and say the syntax should just stay as it is. Most operations don't have their operands in parentheses and have their mandatory attributes as keywords. Introducing a less common syntax breaks the reading flow and unnecessarily anchors attention. The only reason why I think the current syntax may give the impression of the predicate being an operand is the comma between the predicate and the operand: arith.cmpi eq, %a, %b as opposed to arith.cmpi eq %a, %b. I also think that even rudimentary familiarity with the IR structure is sufficient to understand that the predicate is not in fact an operand: operands are values, and values are always prefixed with a % sign.

I'm with @ftynse here. The value that this change is bringing is subjective and I'm sure different people would have different opinions (why not arith.cmpi %a eq %b?). +1 to leaving it as is.

On the other hand, the amount of churn required to essentially remove a comma is scary. For me, it's not worth it.

I understand where you're coming from ( ;) ). I may be alone in this opinion, but I'm not comfortable with the idea that we shouldn't do things just because of churn. Over a comma (and a few braces), however, I'm willing to relent.

Downstreams are a highly valuable part of our community and, whereas MLIR has enjoyed a certain level of flexibility when it comes to IR changes, it has also been brought up multiple times that MLIR is reaching a level of stability and adoption where the impact of these changes should be taken more into consideration. Perhaps we should bring this kind of changes to Discourse to get wider consensus? Idk.

Personally, I'm +1 on cleaning things up but -1 that this is an actual improvement in consistency and legibility (based on a cursory review of the prevailing style). I'm with ftynse's interpretation, and while I could see the argument for removing the comma, I don't think that is an improvement either.

Personally, I'm +1 on cleaning things up but -1 that this is an actual improvement in consistency and legibility (based on a cursory review of the prevailing style). I'm with ftynse's interpretation, and while I could see the argument for removing the comma, I don't think that is an improvement either.

(happy to change my opinion, but I'd prefer to do so based on not just another opinion but some analysis or style guide, which I don't think exists)

I am not conviced that this is an improvement but this is highly subjective ofc.

As @stellaraccident said a style guide is a solution.
We should have the style guide first to avoid changing this back and forth.

herhut added a comment.Sep 2 2022, 3:19 AM

I am going to go against the majority opinion here and say the syntax should just stay as it is. Most operations don't have their operands in parentheses and have their mandatory attributes as keywords. Introducing a less common syntax breaks the reading flow and unnecessarily anchors attention. The only reason why I think the current syntax may give the impression of the predicate being an operand is the comma between the predicate and the operand: arith.cmpi eq, %a, %b as opposed to arith.cmpi eq %a, %b. I also think that even rudimentary familiarity with the IR structure is sufficient to understand that the predicate is not in fact an operand: operands are values, and values are always prefixed with a % sign.

Yeah, now that you mention it, it really is just the comma. I kind of like your suggestion better.

On the other hand, the amount of churn required to essentially remove a comma is scary. For me, it's not worth it.

I understand where you're coming from ( ;) ). I may be alone in this opinion, but I'm not comfortable with the idea that we shouldn't do things just because of churn. Over a comma (and a few braces), however, I'm willing to relent.

Thank you @ftynse for the analysis. I agree that we should not avoid improvements or fixes just because they mean a lot of churn and, I'd say, we typically don't. This is a very expensive , though and the technical debt is fairly small, so I'd be opposed to go forward with this.

We should take this as a reminder that it is important to carefully think about syntax when adding operations.

This seems to have gone beyond the simple unambiguous improvement and reached the level of "it needs an RFC".
Can you please start a discussion on discourse to capture the different viewpoints?

ftynse added a comment.Sep 2 2022, 6:11 AM

I understand where you're coming from ( ;) ). I may be alone in this opinion, but I'm not comfortable with the idea that we shouldn't do things just because of churn. Over a comma (and a few braces), however, I'm willing to relent.

I do agree with you that we shouldn't stop improvements because they cause churn (FWIW, I am no stranger to changes that make downstreams unhappy). But the benefit of improvement should outweigh the costs, hence my comment that this specific change is not worth it.

ftynse added a comment.Sep 2 2022, 6:16 AM

We should take this as a reminder that it is important to carefully think about syntax when adding operations.

I am afraid it was me who added this syntax in the first place. I vaguely remember fighting with the parser four years ago to remove it but the parser won (it wasn't possible for some reason back in the day).

(sarcastic) So we should stage the removal of the comma by first making it optional, parsed with a warning that "The comma is deprecated and will be removed on Sept 10" to give users time to migrate.

In all seriousness, it was useful gauging the feedback for a relatively minimal change with big user impact. My concern specifically is that if we are in the position of introducing new compare ops (e.g. in new dialects), "keeping it consistent with arith" means adopting a perhaps less-than-optimal syntax. Hence, arith should be updated to a "better" syntax, which other dialects can copy.

(sarcastic) So we should stage the removal of the comma by first making it optional, parsed with a warning that "The comma is deprecated and will be removed on Sept 10" to give users time to migrate.

In all seriousness, it was useful gauging the feedback for a relatively minimal change with big user impact. My concern specifically is that if we are in the position of introducing new compare ops (e.g. in new dialects), "keeping it consistent with arith" means adopting a perhaps less-than-optimal syntax. Hence, arith should be updated to a "better" syntax, which other dialects can copy.

Ftr - I am open to an improvement but a) legitimately don't think there is anything wrong with the current syntax (weak opinion), and b) think we have a higher bar on the rationale for such things beyond what I saw in the diff (strong opinion).

Further, for these matters of style/taste, I think that barring a rationale (which can be consistency based like you point out) or strong opinions that one form is better, we should bias towards what is. That is how mature projects operate, ime, and the decision making process working something like this is kind what I'm looking for.

We don't have any of this written down, and maybe we should. Thus my comment about a style guide -- the traditional way to codify both specifics and rationale for such things.

Further, for these matters of style/taste, I think that barring a rationale (which can be consistency based like you point out) or strong opinions that one form is better, we should bias towards what is. That is how mature projects operate, ime, and the decision making process working something like this is kind what I'm looking for.

We don't have any of this written down, and maybe we should. Thus my comment about a style guide -- the traditional way to codify both specifics and rationale for such things.

My two cents, for what is worth. 100% in favour of some sort of style guide. It already came up a couple of times in the past for me. I find consistency in syntax very valuable, it makes dialects predictable and easier to understand. In an ideal world, whenever I see an op in a dialect I haven't worked with before, I should have a decent idea of what it does. You can see some syntax convergence, but also exceptions. It's annoying. I'm not sure if it's too late to create something sensible at this point, or even if it's possible in something of MLIR's nature, but I think it's worth considering. Naming conventions for ops, types, and attributes, operand order, keywords for different types of operations (like the into and from in data movement operations).

I offer myself as volunteer to start some sort of draft collecting common practices in the core dialects, if it's any help. It doesn't even need to be prescriptive, it might be valuable as a reference for people starting a new dialect 🤷Thoughts?

Further, for these matters of style/taste, I think that barring a rationale (which can be consistency based like you point out) or strong opinions that one form is better, we should bias towards what is. That is how mature projects operate, ime, and the decision making process working something like this is kind what I'm looking for.

We don't have any of this written down, and maybe we should. Thus my comment about a style guide -- the traditional way to codify both specifics and rationale for such things.

My two cents, for what is worth. 100% in favour of some sort of style guide. It already came up a couple of times in the past for me. I find consistency in syntax very valuable, it makes dialects predictable and easier to understand. In an ideal world, whenever I see an op in a dialect I haven't worked with before, I should have a decent idea of what it does. You can see some syntax convergence, but also exceptions. It's annoying. I'm not sure if it's too late to create something sensible at this point, or even if it's possible in something of MLIR's nature, but I think it's worth considering. Naming conventions for ops, types, and attributes, operand order, keywords for different types of operations (like the into and from in data movement operations).

I offer myself as volunteer to start some sort of draft collecting common practices in the core dialects, if it's any help. It doesn't even need to be prescriptive, it might be valuable as a reference for people starting a new dialect 🤷Thoughts?

I would love to see a style guide... even an attempt at a draft would be extremely useful, I think. I have personally found it quite hard when introducing new ops to align with what are, in effect, unbounded stylistic opinions. I want our IR to be consistent and read well. I don't want to argue that point by point and patch by patch on phabricator.

I don't know what kind of time commitment you have in mind, but it seems like some bootstrapping of such a doc would be a great thing, and I'd be appreciative to whoever started it.

Mogball abandoned this revision.Jan 8 2023, 1:07 PM