Page MenuHomePhabricator

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

Authored by wsmoses on Jan 13 2022, 3:03 PM.

Details

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.

Diff Detail

Event Timeline

wsmoses created this revision.Jan 13 2022, 3:03 PM
wsmoses requested review of this revision.Jan 13 2022, 3:03 PM
rriddle requested changes to this revision.Jan 13 2022, 3:06 PM
rriddle added inline comments.
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.

This revision now requires changes to proceed.Jan 13 2022, 3:06 PM
rriddle added inline comments.Jan 13 2022, 3:08 PM
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 added inline comments.Jan 13 2022, 3:11 PM
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 added inline comments.Jan 13 2022, 3:13 PM
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 added inline comments.Jan 13 2022, 3:18 PM
mlir/lib/IR/BuiltinTypes.cpp
140

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

wsmoses updated this revision to Diff 399815.Jan 13 2022, 3:36 PM

Remove references to LLVM and fix variable style

wsmoses marked 4 inline comments as done.Jan 13 2022, 3:37 PM
rriddle added inline comments.Jan 18 2022, 10:28 PM
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 updated this revision to Diff 409281.Feb 16 2022, 8:31 AM
wsmoses marked 7 inline comments as done.

Address comments

Address comments

wsmoses marked an inline comment as done.Feb 17 2022, 9:46 AM

bumping @rriddle can you re-review?

rriddle accepted this revision.Feb 23 2022, 9:14 AM
rriddle added inline comments.
mlir/test/Dialect/Arithmetic/canonicalize.mlir
863

What is this rdar referencing?

This revision is now accepted and ready to land.Feb 23 2022, 9:14 AM
wsmoses updated this revision to Diff 410852.Feb 23 2022, 9:27 AM

Remove rdar

mlir/test/Dialect/Arithmetic/canonicalize.mlir
863

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

This revision was automatically updated to reflect the committed changes.
max-kudr added inline comments.
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 added inline comments.Mar 1 2022, 7:34 AM
mlir/lib/Dialect/Arithmetic/IR/ArithmeticOps.cpp
1597–1598

Created fix ready to commit: D120730