[MLIR][Arith] Canonicalize cmpf(int to fp) to cmpi

[wsmoses]

Summary

Given a cmpf of either uitofp or sitofp and a constant, attempt to canonicalize it to a cmpi.

This PR rewrites equivalent code within LLVM to now apply to MLIR arith.

[rriddle]

mlir/lib/IR/BuiltinTypes.cpp

140

I'm not really comfortable referencing LLVM IR from inside of MLIR IR like this, I'd rather have proper documentation here that describes whatever the invariants are.

[rriddle]

mlir/lib/Dialect/Arithmetic/IR/ArithmeticOps.cpp

1408

A lot of this is using LLVM coding style (e.g. CamelCase), can you clean it up? It's fine to "take inspiration" from other parts of the codebase, but we should make sure to clean them up and modernize them. Ideally we don't want to proliferate old things forward that we don't need to.

[rriddle]

mlir/lib/Dialect/Arithmetic/IR/ArithmeticOps.cpp

1402

I would remove this. If either this or the other get out of sync (for some number of reasons), this comment is just confusing. Ideally all of the code here should be properly documented and lay out whatever invariants are driving the transformation. I wouldn't constrain/limit whatever we do here with what has been done in InstCombine.

[wsmoses]

mlir/lib/IR/BuiltinTypes.cpp

140

Honestly, here I'd prefer this to be implemented within fltSemantics (https://llvm.org/doxygen/structllvm_1_1fltSemantics.html) since this is already defined as precision. Unfortunately, the definition of fltSemantics isn't exposed outside APFloat and requires a core change to LLVM.

Assuming the core LLVM change isn't tenable, what would you suggest here: just removing the comment like getWidth (which also has the relevant comment in the header)?

[rriddle]

mlir/lib/IR/BuiltinTypes.cpp

140

You should be able to access the precision of an fltSemantics via APFloat::semanticsPrecision AFAICR.

[wsmoses]

Remove references to LLVM and fix variable style

[rriddle]

mlir/lib/Dialect/Arithmetic/IR/ArithmeticOps.cpp

1428–1429

1432–1436

These look the same value, is there value to having them separate? If so, I would just assign this one to intWidth.

1446–1448

This if condition is a bit brain hurty, can you restructure this to read better?

1452–1455

nit: Please avoid floating statements in control flow.

1490

Can you add parameter comments here (and just below)? It isn't obvious what the true and 1 mean here.

1536

Same comments about param comments here and throughout.

1569

Can you split out some of these logic blocks into helper functions? This function is quite large and covers a lot of different cases.

mlir/test/Dialect/Arithmetic/canonicalize.mlir

803

Please indent the check lines, and can you also avoid using raw SSA values?

https://mlir.llvm.org/getting_started/TestingGuide/

Same for the other tests.

[wsmoses]

Address comments

[wsmoses]

Address comments

[wsmoses]

bumping @rriddle can you re-review?

[rriddle]

mlir/test/Dialect/Arithmetic/canonicalize.mlir

863

What is this rdar referencing?

[wsmoses]

Remove rdar

mlir/test/Dialect/Arithmetic/canonicalize.mlir

863

It's a remant from the LLVM version of these test, it is now removed.

[max-kudr]

mlir/lib/Dialect/Arithmetic/IR/ArithmeticOps.cpp

1597–1598

We are getting warning here. I am sort of lost at how to "future proof" this switch statement without removing default label. Could you please fix this warning?

view on buildbot

/vol/worker/mlir-nvidia/mlir-nvidia/llvm.src/mlir/lib/Dialect/Arithmetic/IR/ArithmeticOps.cpp:1597:9: warning: default label in switch which covers all enumeration values [-Wcovered-switch-default]

[max-kudr]

mlir/lib/Dialect/Arithmetic/IR/ArithmeticOps.cpp

1597–1598

Created fix ready to commit: D120730