dialects: (arith) Use the SameOperandsAndResultType trait in arith dialect

[compor]

This PR:

• Uses the SameOperandsAndResultsType trait in the relevant arith ops
• Cleans up unused test infrastructure for the return type of the above

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.24%. Comparing base (ab5e9bb) to head (a8f92ec).
Report is 16 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3824      +/-   ##
==========================================
- Coverage   91.25%   91.24%   -0.02%     
==========================================
  Files         461      461              
  Lines       57557    57629      +72     
  Branches     5549     5567      +18     
==========================================
+ Hits        52523    52581      +58     
- Misses       3609     3620      +11     
- Partials     1425     1428       +3     

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

[alexarice]

I'm not sure this does much, we already enforce (too strictly) that the operands and results must be the same if my understanding is correct

[compor]

I'm not sure this does much, we already enforce (too strictly) that the operands and results must be the same if my understanding is correct

It's the same as in #3761 and here's an earlier relevant discussion #3751 (comment)

[alexarice]

Right, I would have made the same comment on the other one if I had got round to reviewing it. I believe the operation is already more constrained by the current constraints.

[compor]

Right, I would have made the same comment on the other one if I had got round to reviewing it. I believe the operation is already more constrained by the current constraints.

Sorry I don't understand, even for shapes?

[alexarice]

I believe the current constraint enforces that the types (including shapes) are exactly the same.

[alexarice]

Maybe a better question is do you have a test case which has different behaviour with this change?

[compor]

Maybe a better question is do you have a test case which has different behaviour with this change?

Maybe I wasn't clear, but my annoyance for what I'm trying out here is that it is overly constrained, especially with dynamic shapes:

  %0 = "test.op"() : () -> tensor<1x?xi64>
  %1 = "test.op"() : () -> tensor<1x3xi64>
  %2 = "arith.addi"(%0, %1) <{overflowFlags = #arith.overflow<none>}> : (tensor<1x?xi64>, tensor<1x3xi64>) -> tensor<1x?xi64>

This passes from mlir-opt but not us ATM.

My idea was to add the attribute and then relax this.
I didn't do it in math dialect, because I don't care (yet?).

If there is a way to do this with constraints, I'll gladly drop this.

[superlopuh]

This is a great example, could it be added to this PR?

[superlopuh]

Might as well relax in one go, no?

[compor]

Might as well relax in one go, no?

Yeah, sure, wdyt @alexarice ?

[alexarice]

I agree, with the caveat that I'm almost certain this will mess up the assembly format in a way I don't know how to fix

[alexarice]

what would the custom format look like for your example, as arith.addi only have one type in its assembly format

[compor]

what would the custom format look like for your example, as arith.addi only have one type in its assembly format

%0 = "test.op"() : () -> tensor<1x?xi64>
%1 = "test.op"() : () -> tensor<1x3xi64>
%2 = arith.addi %0, %1 : tensor<1x?xi64

We get a different error then, because it assumes that %1 should be of type tensor<1x?xi64> (the return type?)

operand is used with type tensor<1x?xi64>, but has been previously used or defined with type tensor<1x3xi64>

which I find even more confusing.

[alexarice]

I just tried it myself, mlir-opt can't roundtrip arith.addi. How cursed.

 ~/p/mlir   master ±  mlir-opt ../mlir/test.mlir
module {
  %0 = "test.op"() : () -> tensor<1x?xi64>
  %1 = "test.op"() : () -> tensor<1x3xi64>
  %2 = arith.addi %0, %1 : tensor<1x?xi64>
}

 ~/p/mlir   master ±  mlir-opt ../mlir/test.mlir --mlir-print-op-generic
"builtin.module"() ({
  %0 = "test.op"() : () -> tensor<1x?xi64>
  %1 = "test.op"() : () -> tensor<1x3xi64>
  %2 = "arith.addi"(%0, %1) <{overflowFlags = #arith.overflow<none>}> : (tensor<1x?xi64>, tensor<1x3xi64>) -> tensor<1x?xi64>
}) : () -> ()

 ~/p/mlir   master ±  mlir-opt ../mlir/test.mlir | mlir-opt --mlir-print-op-generic
<stdin>:4:23: error: use of value '%1' expects different type than prior uses: 'tensor<1x?xi64>' vs 'tensor<1x3xi64>'
  %2 = arith.addi %0, %1 : tensor<1x?xi64>
                      ^
<stdin>:3:3: note: prior use here
  %1 = "test.op"() : () -> tensor<1x3xi64>
  ^