Page MenuHomePhabricator

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

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



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.

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

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

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

Honestly, here I'd prefer this to be implemented within fltSemantics ( 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

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

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


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


nit: Please avoid floating statements in control flow.


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


Same comments about param comments here and throughout.


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


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

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.

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


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.

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

Created fix ready to commit: D120730